-
Notifications
You must be signed in to change notification settings - Fork 20
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
for update-stemcell, only download releases if necessary #493
Conversation
We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story. The labels on this github issue will be updated when the story is started. |
db5007c
to
81a933d
Compare
f694c91
to
10df70e
Compare
06e88ed
to
2d44f25
Compare
2d44f25
to
baceadb
Compare
baceadb
to
2c3d769
Compare
2c3d769
to
e13a06d
Compare
Signed-off-by: Daniel Linsley <[email protected]>
e13a06d
to
bc50af4
Compare
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.
Change looks good and I agree that defaulting this behavior rather than requiring a flag is correct.
Afaict there isn't a test that verifies the behavior change in update_stemcell.go itself. If I'm right, would you mind adding one?
lock.RemotePath = remote.RemotePath | ||
lock.RemoteSource = remote.RemoteSource | ||
lock.SHA1 = remote.SHA1 | ||
if remote.SHA1 == "" || remote.SHA1 == "not-calculated" { |
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.
Where is "not-calculated" from? Maybe pull this out into a const and add a note on what emits this value. If there was a test, that would also solve this question.
(It was cool to see the PR review request. I miss y'all. Hope you're all 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.
The s3
and github
sources can set this as not-calculated
particularly when no-download specified.
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 added a comment. I don't recall anything that would break from this change.
This issue from 2021 is kinda related. We only calculate the SHA1 because we used to have a huge s3 bucket that did not have object checksums enabled and adding it would have been super computationally expensive. Funnily, if we had enabled that feature we would have saved thousands of release downloads.
4e35f80
to
f8cd1fc
Compare
The updated tests add a third release to the tile under test that provides sufficient metadata to the update-stemcell method to not downloaded the release. Updated the returned metadata of the existing releases in the tile to explicitly return either empty string or |
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.
Appreciate the updates and work! Approving and merging 🙏
This broke our pipelines. It is not downloading the releases in our compilation manager. I am going to revert this PR and re-release kiln. |
Sorry about that, didn't realize the download was desired for this command, may put this behind a --no-download flag instead |
implements #492
GetMatchedRelease
to include remote SHA1 of matched release from remoteGetMatchedRelease
to include remote SHA1 of matched release from remote