Skip to content
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

Add Compatible Kubernetes to Version Printout #2446

Merged
merged 14 commits into from
Mar 27, 2020

Conversation

theishshah
Copy link
Member

@theishshah theishshah commented Jan 21, 2020

Description of the change:
Adding necessary fields to the verison, needs implementation on build side to populate.

The build implementation will involve passing along the required values in the ldflags field

Closes #2118

@theishshah theishshah requested a review from estroz January 21, 2020 18:37
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 21, 2020
version/version.go Outdated Show resolved Hide resolved
Version = "v0.14.0+git"
GitVersion = "unknown"
GitCommit = "unknown"
KubernetesVendorVersion = "unknown"
Copy link
Member

@estroz estroz Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we technically support a range of k8s versions, it would be informative to print min and max k8s version variables:

$ operator-sdk version
operator-sdk version: "v0.14.0", commit: ..., minKubernetesVersion: "v1.14.0", maxKubernetesVersion: "v1.16.3"

This will reflect the eventual test and compatibility matrices we will be adding as follow-ups. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we effectively trying to convey the information in the client-go compatibility matrix?

The problem with min and max versions is that the answer to the question "is my code compatible with my kubernetes server?" is "yes if you're using the same client and server version, maybe if your client and server versions differ by exactly 1, and otherwise there are no guarantees"

Because of those nuances, I would suggest that we just list our client-go dependency and add a link to our dependencies section over to the client-go compatibility matrix? WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that was the intent. The version from the client-go kubernetes-x.y.z tag plus a link is a better approach to capture this information.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We technically do support a range of k8s versions (i.e client-go matrix) but we don't really test against supposedly compatible k8s versions.

So I agree that linking to the client-go matrix as an indirect indication of compatibility is okay but probably not listing out the exact versions here since we haven't tested and guaranteed that the SDK works against all of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So right now this would just be the k8s version in our CI K8S_VERSION.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that sounds good to me.

Ideally (possibly for a follow-on) we would enforce in CI that K8S_VERSION aligns with our client-go dependency. How doable would that be?

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever we decide to include, we should auto-populate via ldflags from the Makefile. We'll need to add that before merging this.

version/version.go Outdated Show resolved Hide resolved
Version = "v0.14.0+git"
GitVersion = "unknown"
GitCommit = "unknown"
KubernetesVendorVersion = "unknown"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we effectively trying to convey the information in the client-go compatibility matrix?

The problem with min and max versions is that the answer to the question "is my code compatible with my kubernetes server?" is "yes if you're using the same client and server version, maybe if your client and server versions differ by exactly 1, and otherwise there are no guarantees"

Because of those nuances, I would suggest that we just list our client-go dependency and add a link to our dependencies section over to the client-go compatibility matrix? WDYT?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2020
@@ -31,7 +31,7 @@ func NewCmd() *cobra.Command {
if version == "unknown" {
version = ver.Version
}
fmt.Printf("operator-sdk version: %q, commit: %q, go version: %q\n", version, ver.GitCommit, ver.GoVersion)
fmt.Printf("operator-sdk version: %q, commit: %q, kubernetes version: %q, go version: %q, bulid date: %q\n", version, ver.GitCommit, ver.KubernetesVendorVersion, ver.GoVersion)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fmt.Printf("operator-sdk version: %q, commit: %q, kubernetes version: %q, go version: %q, bulid date: %q\n", version, ver.GitCommit, ver.KubernetesVendorVersion, ver.GoVersion)
fmt.Printf("operator-sdk version: %q, commit: %q, kubernetes version: %q, go version: %q\n", version, ver.GitCommit, ver.KubernetesVendorVersion, ver.GoVersion)

Version = "v0.14.0+git"
GitVersion = "unknown"
GitCommit = "unknown"
KubernetesVendorVersion = "unknown"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just KubernetesVersion to align with what we output with operator-sdk version?

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@hasbro17
Copy link
Contributor

hasbro17 commented Mar 9, 2020

Fixes #2118

While we can't have a compatibility matrix without actually testing the matrix first in CI, this should at least specify compatibility for a single version.

@theishshah theishshah changed the title [WIP] Add Compatible Kubernetes to Version Printout Add Compatible Kubernetes to Version Printout Mar 26, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2020
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

although I don't think this closes #2118 since we don't add what the issue author is asking for here.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2020
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joelanford
Copy link
Member

@theishshah Can you also add a line to the CHANGELOG?

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@theishshah theishshah merged commit e02c139 into operator-framework:master Mar 27, 2020
@theishshah theishshah deleted the k8sversion-print branch March 27, 2020 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do you have a Kubernetes supported release matrix?
6 participants