-
Notifications
You must be signed in to change notification settings - Fork 110
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
[kubetest2-gke deployer] Automatically resolves the release channel if it's not specified #142
[kubetest2-gke deployer] Automatically resolves the release channel if it's not specified #142
Conversation
c5ad1e8
to
80649e1
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.
Let's make this best-effort instead of a run failing error.
Otherwise lgtm.
Isn't None channel usually a superset of all channels except maybe Rapid?
kubetest2-gke/deployer/version.go
Outdated
} | ||
|
||
func validateReleaseChannel(releaseChannel string) error { | ||
if releaseChannel != "" && !validReleaseChannels.Has(releaseChannel) { |
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.
maybe can reuse toReleaseChannel
to avoid needing sets.
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.
toReleaseChannel
is used to convert the channel name in the server config to the channel name in the gcloud
flag, so it cannot be used here. I have changed to simple array iteration to avoid using sets.
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 meant since the strings are the same, we can use toLower
/toUpper
and the default case to also use it as a validation method.
But considering it's just 4 values the array solution is fine as well.
80649e1
to
280fd57
Compare
280fd57
to
732c00e
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
/approve
/hold
couple of nits feel free to cancel the hold.
{ | ||
desc: "the same major.minor version is a match", | ||
version: "1.19", | ||
target: "1.19.10-gke.1000", |
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.
nit:
also add a case with version
and target
swapped to exercise
if len(parts) < len(matchParts) {
return false
}
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'll address this comment in my next PR.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amwat, chizhg 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 |
/unhold |
Sometimes when the users create the GKE clusters, they just want to specify a valid cluster version but do not want to hard code the channel, but if the version is only valid in a specific channel (e.g. rapid), the cluster creation will fail.
This PR adds the support to automatically resolve the release channel if it's not specified, so that as long as the specified version is in any of the valid release channels,
kubetest2 gke --up
will work./cc @amwat