-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Drop unnecessary redirection of ci/latest.txt to ci-cross/latest.txt for kubeadm's kubernetes version flag #63504
Conversation
cc @ixdy |
/test pull-kubernetes-integration |
Nothing is configured to try to use latest-1.11 yet, right? |
@ixdy correct. AFAIK |
/test pull-kubernetes-e2e-gce |
/milestone v1.11 |
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.
/approve
/lgtm
@ixdy can we get rid of this inconsistency with |
cmd/kubeadm/app/util/version.go
Outdated
@@ -123,7 +123,14 @@ func splitVersion(version string) (string, string, error) { | |||
switch { | |||
case strings.HasPrefix(subs[0][2], "ci"): | |||
// Special case. CI images populated only by ci-cross area | |||
urlSuffix = "ci-cross" | |||
if subs[0][3] == "latest" { |
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 will break ci/latest-1
and ci/latest-1.11
(versions in master branch). it will now be pointing to images that doesn't exists in registry.
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.
Does anyone use ci/latest-1.11 or just latest-1.11 ?
I don't know of any tooling that uses ci/latest other then the test infra.
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.
@kad you mentioned on slack
I still suggest to simplify it to `urlSuffix = subs[0][2]` in all cases where bucket starts with “ci” and update in test-infra config.json to use ci-cross/latest where it is applicable explicitly.
So i'll do revise the PR with your suggestion
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.
@timothysc latest-1.11
is usable for official images, v1.11.0-alpha.2
at the moment. ci/latest-1.11
points to tip of the branch builds, and images for this build are not available at the moment until corresponding ci-cross build finishes. Yes, primary user is test-infra, and there it will be good to have explicit pointer which build is used for testing (cross vs. standard build).
c82fbfa
to
2ac8989
Compare
cmd/kubeadm/app/util/version_test.go
Outdated
@@ -180,8 +180,10 @@ func TestSplitVersion(t *testing.T) { | |||
{"release/v1.7.0", "https://dl.k8s.io/release", "v1.7.0", true}, | |||
{"release/latest-1.7", "https://dl.k8s.io/release", "latest-1.7", true}, | |||
// CI builds area, lookup actual builds at ci-cross/*.txt | |||
{"ci/latest", "https://dl.k8s.io/ci-cross", "latest", true}, |
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 test will fail, if I am not mistaken.
We should stop special casing "ci-cross" and just use the configuration in test-infra to dictate where we pick up the builds from. For 1.8,1.9,1.10 branches, we need to load the latest version from ci/latest*.txt. For master, 1.11 etc, we need to load from ci-cross/latest*.txt. We need to update test-infra configs if things fail. try these to verify: ``` gsutil cat gs://kubernetes-release-dev/ci/latest-1.9.txt gsutil cat gs://kubernetes-release-dev/ci-cross/latest.txt ```
2ac8989
to
ff26e57
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, kad, timothysc 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 |
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process Pull Request Labels
|
Automatic merge from submit-queue (batch tested with PRs 62850, 63504). If you want to cherry-pick this change to another branch, please follow the instructions here. |
…pstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #63504: Improve where we load builds from for kubeadm upgrade jobs Cherry pick of #63504 on release-1.10. #63504: Improve where we load builds from for kubeadm upgrade jobs ```release-note Fixed where we get latest builds for stable branches ```
…pstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #63504: Improve where we load builds from for kubeadm upgrade jobs Cherry pick of #63504 on release-1.9. #63504: Improve where we load builds from for kubeadm upgrade jobs ```release-note Fixed where we get latest builds for stable branches ```
@dims Can you please write a more descriptive title and release note? |
@luxas updated. if you have a suggestion to make it even better, please let me know. |
@dims Thanks, that's very descriptive 👍 |
Is this only for Google Cloud? I was wondering whether this works for AWS and other cloud as well. |
What this PR does / why we need it:
For 1.8,1.9,1.10 branches, we need to load the latest
version from ci/latest*.txt.
For master, we need to load the version number from
ci-cross/latest.txt
try these to verify:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #61483
Special notes for your reviewer:
Release note: