-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CVE feed: comply with the JSON feed specifications and add the full JSON feed object in the script output to add last_updated
root fields
#76
Conversation
@mtardy thank you so much for the updates!!! If you have resolved the suggestions from #75 already in this PR, we can close the #75 without merging and then include all the changes from that PR in this one which we will merge along with kubernetes/website#38579 Does that sound like a reasonable next step? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some really minor suggestions otherwise looks good.
I will review kubernetes/website#38579 later next when I get a chance
071e7bd
to
ef13af4
Compare
last_updated
root fields
/lgtm (@mtardy please unhold when |
/label tide/merge-method-squash |
I think it might be helpful not to squash merge this one. Instead, @mtardy, you can squash this locally and force-push. |
I also think it's helpful to keep fac4ebd (Comply with JSON feed specifications) as its own commit. |
Yes sure, It seems that some commits might be better on their own. |
Simplify the output by printing to STDOUT and using tee to duplicate the STDOUT in a file in the bash script. Fix typos in comments and add explanations about usage of tee Co-authored-by: Pushkar Joglekar <3390906+PushkarJ@users.noreply.github.com>
Honestly, I would just remove the last commit ef13af4 and squash it. |
ef13af4
to
1a3c241
Compare
/remove-label tide/merge-method-squash |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtardy, PushkarJ The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
🥳 |
This PR bundles another PR #75. All the changes were included in this one to make it simpler.
This is making modifications to the output of the CVE feed scripts and thus should be synchronized with the merge of an according PR on the k/website side. This is the PR on the k/website site: kubernetes/website#38579.
This is the content of PR #75:
Fixes kubernetes/website#36808.
I simplified the code logically and added documentation on why it's parsing the way it's parsing.
Anecdotally I change all double quotes to single quotes.
I took some arbitrary decisions:
cve_id
into existing fieldid
;issue_url
into existing fieldurl
;cve_url
into existing fieldexternal_url
;_kubernetes_io
as custom extension;number
intoissue_number
for clarity.I didn't add it because it was maybe too much but it would be trivial to add optional fields
date_published
anddate_modified
using GitHub issues metadata.The CVE feed looks similar to this after the fix (I only used the two last CVEs for the example, note that the indention is not exact):
On a side and useless note, is it useful that the scripts output the JSON dumps? Otherwise, I think it would be more elegant that it just prints to STDOUT the result and that the other bash scripts
python3 script.py > the_file.json
. But this one is really a nit!This is the old content of this PR:
This resolves #72.
I don't know if it's the best way to proceed but this PR includes the commits from #75, so this should be merged into this PR or better, after!
Identically, it needs some changes on the k/website side.
/hold
So the unique commit this PR brings is e61ffa9.
I took the liberty to name it
updated_at
, but we have plenty of possibilities here and maybe we can find something better? Or more in line with the Kubernetes API? I don't know./cc @PushkarJ @nehaLohia27