-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Update CVE feed layouts for new JSON feed format #38579
Conversation
/cc @PushkarJ @nehaLohia27 |
/hold (AIUI) this change needs to land alongside the matching change to Kubernetes |
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.
Optionally, check that "version": "https://jsonfeed.org/version/1.1"
in the fetched feed, and fail the build if this doesn't match.
We thought with @PushkarJ that it would be better to put the caption at the top of the table in order for it to be more noticeable. I took the liberty to add a property of CSS directly in the shortcode HTML template (since the class specified seems not to exist). |
Yes! Why not, I added this stuff 32442ff. |
The build error is:
|
Make sure that the upstream feed includes that change! |
Ah you mean the actual build error at the moment? Yes the PRs need each others to work, we have to merge them simultaneously. The best would be to update the script first, it will error the website build when properly generated and then to update the website for the HTML page to be generated correctly. |
LGTM label has been added. Git tree hash: 2b3c949c61c8e8a1f1e066cdfc228dc1ff979e73
|
/remove-label tide/merge-method-squash It's useful to note this history. If there were 22 commits I'd think differently, mind. |
/lgtm |
LGTM label has been added. Git tree hash: 6a3bbadf9318fb10e5fff6ec43bd292c73e1e6ea
|
@mtardy . Are you waiting for another pull request to merge? |
@kbhawkey Not really, I would need someone to approve this one, even though it's currently not building, here's why: |
+1 to what @mtardy said |
[cve_url] | ||
other = "CVE URL" | ||
[cve_table_date_before] | ||
other = "(last updated: " |
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.
This is a bit unusual
I'll look at how you are constructing the string
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.
I was thinking on the contrary that it was how we usually do these things, but we can do whatever you think is better.
Lines 275 to 279 in 206231d
[release_date_after] | |
other = ")" | |
[release_date_before] | |
other = "(released: " |
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.
I prefer to have the localized string be a format string:
[cve_table_date_with_label]
other = "(last updated: %s)"
and then use a string formatter to include the localized date. That's equivalent, but the placeholder makes it easier for localization teams to understand.
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.
I'll update this in the follow-up PR #39513 :)
@@ -1,19 +1,23 @@ | |||
{{ $feed := getJSON .Site.Params.cveFeedBucket }} | |||
{{ if ne $feed.version "https://jsonfeed.org/version/1.1" }} | |||
{{ errorf "Build Failed. CVE feed does not comply with JSON feed v1.1" }} |
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.
Would be better if the build did not fail, but instead use a 'placeholder page' and send an alert (or something like that)
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.
Ok makes sense. This is a bit in contradiction with this previous review #38579 (review), @sftim do you have an opinion on that as well?
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.
Sounds like a nice enhancement; however, I would save that for a follow-up PR.
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.
Yep we'll look at this here #39513 next!
Hi, @mtardy . Anyway, the currently published page is: /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kbhawkey 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 |
The workflow is the following: a script in kubernetes/sig-security is picked up by a job in k8s infra, scheduled to run every 2 hours, and the website retrieves the file created by this job to update its internal JSON data, required to build this page. I just merged the script side, the next job should run at 11:09 CET and then the preview of this PR should work, and the old live page will fail if I do not merge this new one once it picks up the new JSON :) |
I tested locally and the web page works with the new output format, let's merge. |
Does this work on website? |
7ddf310
to
3305d7b
Compare
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Related to the following discussion kubernetes#38579 (comment)
Previously it would cause the build to fail, which could lead to confusing situation since the CVE feed comes from outside of the website and could break the workflow. See related discussion: kubernetes#38579 (comment)
Previsouly we were using two data items: ```toml [cve_table_date_before] other = "(last updated: " [cve_table_date_after] other = ")" ``` Which was simplified, using printf, to: ```toml [cve_table_date_format_string] other = "(last updated: %s)" ``` This is related to the following discussion kubernetes#38579 (comment)
Previously it would cause the build to fail, which could lead to confusing situation since the CVE feed comes from outside of the website and could break the workflow. See related discussion: kubernetes#38579 (comment)
Previsouly we were using two data items: ```toml [cve_table_date_before] other = "(last updated: " [cve_table_date_after] other = ")" ``` Which was simplified, using printf, to: ```toml [cve_table_date_format_string] other = "(last updated: %s)" ``` This is related to the following discussion kubernetes#38579 (comment)
Previously it would cause the build to fail, which could lead to confusing situation since the CVE feed comes from outside of the website and could break the workflow. See related discussion: kubernetes#38579 (comment)
Previsouly we were using two data items: ```toml [cve_table_date_before] other = "(last updated: " [cve_table_date_after] other = ")" ``` Which was simplified, using printf, to: ```toml [cve_table_date_format_string] other = "(last updated: %s)" ``` This is related to the following discussion kubernetes#38579 (comment)
Previously it would cause the build to fail, which could lead to confusing situation since the CVE feed comes from outside of the website and could break the workflow. See related discussion: kubernetes#38579 (comment)
This PR fixes the layout for the CVE feed for the new JSON format, to be merged after kubernetes/sig-security#75 and kubernetes/sig-security#76 are merged. While these are not merged and deployed, the build will fail because the JSON format is different.
last_updated
root fields sig-security#76 adds the_kubernetes_io.updated_at
root field used to display the last update date in the caption of the CVE table.Note that PR 75 was merged into PR 76 that bundles all the changes for simplicity.
Note that
{{ getJSON .Site.Params.cveFeedBucket | jsonify }}
can look useless but it's to make the JSON file looks "unpretty" like previously (with no space and no indentation). If we don't care about this we can just replace it with{{ os.ReadFile .Site.Params.cveFeedBucket }}
. There was some note about this Hugo issue gohugoio/hugo#7229 but I did not encounter this, you can check usingcurl
andgrep <
to see.Testing this PR locally
Generate the new JSON CVE feed file with the new bash and python scripts from PR 75 and 76, or just download this version generated (with only the four latest CVEs in order to reduce the size of the file) and put the file under
static/official-cve-feed.json
.official-cve-feed.json
extract new version according to PR 75 and 76 as of 2022-12-20T15:12:31ZModify
config.toml
to use the local file.Preview
The HTML page looks very similar with just a new caption indicating the last update date.