-
Notifications
You must be signed in to change notification settings - Fork 56
Cache license network operations #213
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
Conversation
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit modifies the license downloader to make use of the cache code previously used to cache individual JSON files. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit modifies the GetLicenses method in the implementation to take a version tag. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This returns the version of the license list Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
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.
How is the version passed from CLI to the downloader ? Or do you have something else in mind to send version info ?
I didn't wire it up to there yet. I wanted to sync with you first on how we would do it considering we will embed one license list. THIS PR lets us do it, but the change it is mostly meant to address caching. |
My guess is that we should by default use the embedded version. And then if we get a tag passed via the cli then we can download that. ALso I was looking at the SBOM and they don't have the license list version, at least not in the JSON version. We need to surface that too. |
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.
/lgtm
if we need to change anything lets do it in a follow up
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, puerco 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces two changes into the license downloader:
cache when loading the licenses. This rewired the old cache previously used to cache json files to cache the license list and latest version tag
This also drops the old single json file download function which is no longer required (
getLicenseFromURL
)Which issue(s) this PR fixes:
Follow-up to #201
Special notes for your reviewer:
/cc @sbs2001
Does this PR introduce a user-facing change?