Skip to content
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

Stream metadata prep #4576

Merged

Conversation

cgwalters
Copy link
Member

cache: Create an internal "urlWithIntegrity" API, use better filename

Prep for openshift/os#477
Clarify internally that this sha256 is uncompressed; the stream
will also have the compressed sha256 which we should support
verifying too (as well as the size).

Since we're now parsing the URL better, use the uncompressed
filename as the target name. This makes it much much more
obvious to users how to manually clean up old images in
the cache. We can do this because for both FCOS/RHCOS
the filenames contain versions and are immutable - we will
never go back and change an image.

@cgwalters
Copy link
Member Author

(I tested this locally w/libvirt btw, works fine)

@cgwalters
Copy link
Member Author

(This can wait for 4.8)

Nothing external uses it; prep for further cleanups.
@cgwalters
Copy link
Member Author

OK, 4.7 is branched; ping for review?
/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@cgwalters: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

OK, 4.7 is branched; ping for review?
/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@patrickdillon
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021
q := parsedURL.Query()
if uncompressedSHA256, ok := q["sha256"]; ok {
u.uncompressedSHA256 = uncompressedSHA256[0]
delete(q, "sha256")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the intention is to remove the query value from parsedURL, this is not going to have that effect.
https://play.golang.org/p/vISRRGvej-w

Also, I think it is preferred to use the Del function.

Suggested change
delete(q, "sha256")
q.Del("sha256")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needed to re-set the RawQuery. Both fixed.

@cgwalters
Copy link
Member Author

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021
Prep for openshift/os#477
Clarify internally that this sha256 is uncompressed; the stream
will also have the compressed sha256 which we should support
verifying too (as well as the size).

Since we're now parsing the URL better, use the uncompressed
filename as the target name.  This makes it much *much* more
obvious to users how to manually clean up old images in
the cache.  We can do this because for both FCOS/RHCOS
the filenames contain versions and are immutable - we will
never go back and change an image.
@staebler
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

17 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@cgwalters
Copy link
Member Author

ovirt is a flake, nothing related to this PR
/override ci/prow/e2e-ovirt

@openshift-ci-robot
Copy link
Contributor

@cgwalters: cgwalters unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

In response to this:

ovirt is a flake, nothing related to this PR
/override ci/prow/e2e-ovirt

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@staebler
Copy link
Contributor

ovirt is a flake, nothing related to this PR
/override ci/prow/e2e-ovirt

/override ci/prow/e2e-ovirt

@openshift-ci-robot
Copy link
Contributor

@staebler: Overrode contexts on behalf of staebler: ci/prow/e2e-ovirt

In response to this:

ovirt is a flake, nothing related to this PR
/override ci/prow/e2e-ovirt

/override ci/prow/e2e-ovirt

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 676655f into openshift:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants