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

Template functions for recommended kubernetes versions #10369

Merged
merged 3 commits into from
Dec 15, 2020

Conversation

olemarkus
Copy link
Member

This PR implements two template functions for setting recommended k8s version based on info from the given channel.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 5, 2020
return content
}

funcs["ChannelRecommendedKubernetesUpgradeVersion"] = func(version string) string {
Copy link
Member

Choose a reason for hiding this comment

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

These function names are pretty long.

This is essentially the recommended patch version for a given kubernetes version and the function below is the recommended minor+patch version for a given kops version.

How about:

ChannelRecommendedKubernetesPatchVersion
ChannelRecommendedKubernetesMinorVersion

or even just

ChannelKubernetesPatchVersion
ChannelKubernetesMinorVersion

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not the easiest functions to name :)

I mainly based the function names after the primary underlying function. I don't mind long function names; it boils down to what most intuitive for users.

@olemarkus
Copy link
Member Author

I realise that channel is actually something we pass in as a value.
I could pass channel name as a param to these template functions, but I think the overall code would be more awkward.
A better alternative may be to make the value of the channel CLI arg available as a function too.

Any thoughts?

@rifelpet
Copy link
Member

rifelpet commented Dec 8, 2020

Yea, I don't have a strong opinion about how these should work or their name though I do see the value in having them. Perhaps your original names are sufficient.

I do think its worthy of some documentation so that users don't have to dig into the source code to find the function names.

I'm also wondering if not specifying --channel should use the default channel or not load any channel and have those functions fail somehow. I suppose it only really impacts airgapped environments since toolbox template used to not make any network requests and now it does by default. If we go with the default channel then we should consider a release note mentioning the change in behavior.

@olemarkus
Copy link
Member Author

My documentation got lost somewhere ... I had actually written that.

Those who run kops in airgapped envs could point to their local channel files. I would guess they have one in order to update their clusters as well.

I'll dig up those docs again, add a bit of release notes, but otherwise keep this PR as is. There is sufficient time to change anything later on. And I have some more similar functions I want to add such as recommended image.

Recommended image would also require cloud to be passed though. Which means either passing that as a variable or as another CLI arg similar to the channel discussion above.

@rifelpet
Copy link
Member

rifelpet commented Dec 8, 2020

Yes, I think all of that sounds good 👍🏻

Airgapped users can use their local channel file but they'll need to know to add the --channel flag pointing to their local file when upgrading to kops 1.20, which is what I think the release notes could help with.

@olemarkus
Copy link
Member Author

Actually saving files to disk seems to make a difference ... docs pushed.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2020
@k8s-ci-robot
Copy link
Contributor

@olemarkus: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kops-e2e-k8s-containerd 673fa24 link /test pull-kops-e2e-k8s-containerd
pull-kops-verify-hashes 673fa24 link /test pull-kops-verify-hashes

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rifelpet
Copy link
Member

This LGTM. Once the conflict is resolved we can get it merged

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2020
@rifelpet
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, rifelpet

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2020
@k8s-ci-robot k8s-ci-robot merged commit 54a5f4e into kubernetes:master Dec 15, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants