-
Notifications
You must be signed in to change notification settings - Fork 153
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
Fix gitops check
#4158
Fix gitops check
#4158
Conversation
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'm ok with this, it works when the previous version doesn't 😄
pkg/services/check/check.go
Outdated
} | ||
|
||
return semver.NewVersion(version) | ||
return fmt.Sprintf("✔ Kubernetes %s %s", sv.String(), kubernetesConstraints), nil |
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.
Minor here, you don't need to use sv.String()
just using sv
will get .String()
called on it by %s
.
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 appreciate that it's not in this PR, but the kubernetesContraints
looks overly broad, I am not sure that Flux supports those versions (and they're not supported upstream)?
) | ||
|
||
const ( | ||
kubernetesConstraints = ">=1.20.6-0" | ||
kubernetesConstraints = ">=1.26" |
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.
🙇🏻♂️
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.
Thanks for fixing the K8s bits!
The `--short` flag has been removed from `kubectl version` in 1.28 (https://github.com/kubernetes/kubernetes/blob/7fe31be11fbe9b44af262d5f5cffb1e73648aa96/CHANGELOG/CHANGELOG-1.28.md#L1718) so the command obviously fails now. This commit changes the behaviour of the `gitops check` command to create a client-go DiscoveryClient and use that to retrieve the server version. That way we don't have to rely on forking a `kubectl` process and the output being consistent. The code is now much cleaner, easier to read and properly tested. closes #4157 Signed-off-by: Max Jonas Werner <mail@makk.es>
The support policy of Weave GitOps is to "test Weave GitOps against the latest supported Kubernetes releases" which means that only 1.26, 1.27 and 1.28 are supported at this point. This change doesn't prevent Weave GitOps from being run on older versions of Kubernetes as the constraint is only used by the `gitops check` command which is purely informational. Signed-off-by: Max Jonas Werner <mail@makk.es>
closes #4157
What changed?
gitops check
command works again.Why was this change made?
--short
flag has been removed fromkubectl version
in 1.28 (https://github.com/kubernetes/kubernetes/blob/7fe31be11fbe9b44af262d5f5cffb1e73648aa96/CHANGELOG/CHANGELOG-1.28.md#deprecation) so the command obviously fails now.How was this change implemented?
This commit changes the behaviour of the
gitops check
command to create a client-go DiscoveryClient and use that to retrieve the server version. That way we don't have to rely on forking akubectl
process and the output being consistent.The code is now much cleaner, easier to read and properly tested.
How did you validate the change?
Unit tests and manual runs.
Release notes
gitops check
command with most recent version ofkubectl
.gitops check
will only yield a positive result if the Kubernetes version is at least 1.26.0.Documentation Changes
n/a