-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Use all kops mirrors to determine artifacts hashes #9958
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman 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 |
/cc @justinsb @mikesplain @rifelpet @zetaab |
9c8e02f
to
1316139
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.
/lgtm
This new logic and the constants look very similar to https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/urls.go would it make sense to consolidate them? |
It would make sense to move this to a function somewhere they both could use, but no idea where. It can also be done in a followup PR once we agree where to extract it. |
1316139
to
3bcce67
Compare
654d8a8
to
3d4bfa6
Compare
/hold cancel |
3d4bfa6
to
0c6f1c7
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.
/lgtm
Current implementation only looks for hash files in the S3 mirror and, if that is down, kops commands will fail with:
The code in the assets remapping and mirroring is quite complex and even if this looks a bit less elegant than I would like, it works pretty well.
Fixes: #9951