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.

Sign up for GitHub

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

Jump to bottom

update_kubernetes_version: automate the update of DefaultKubernetesVersion #9298

Merged
merged 2 commits into from
Sep 23, 2020
Merged

update_kubernetes_version: automate the update of DefaultKubernetesVersion #9298

merged 2 commits into from
Sep 23, 2020

Conversation

prezha
Copy link
Contributor

@prezha prezha commented Sep 22, 2020 •
edited
Loading

fixes #4392 (iteration 0)

description:

existing hack/kubernetes_version/update_kubernetes_version.go script was modified to automate the update of DefaultKubernetesVersion and NewestKubernetesVersion - it now polls GitHub Releases (using github.com/google/go-github) for the latest Kubernetes [pre-]releases and updates the pkg/minikube/constants/constants.go accordingly, whereas:

  • DefaultKubernetesVersion is updated to the latest release version
  • NewestKubernetesVersion is updated to the latest -rc or -beta pre-release version
  • if such NewestKubernetesVersion is less than DefaultKubernetesVersion, then DefaultKubernetesVersion would be used for both
  • make test passes (note: make generate-docs must be run before make test, or TestGenerateDocs test will fail - this dependency/requirement was removed in commit 1444ce3)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 22, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @prezha. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: prezha
To complete the pull request process, please assign tstromberg
You can assign the PR to them by writing /assign @tstromberg in a comment when ready.

The full list of commands accepted by this bot can be found 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

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@prezha
Copy link
Contributor Author

prezha commented Sep 22, 2020

/assign @tstromberg

tstromberg
tstromberg reviewed Sep 22, 2020
View reviewed changes
hack/kubernetes_version/update_kubernetes_version.go Outdated
@@ -77,7 +128,7 @@ func main() {
return err
}
re = regexp.MustCompile(`kubernetesVersion: .*`)
cf = []byte(re.ReplaceAllString(string(cf), "kubernetesVersion: "+v))
cf = []byte(re.ReplaceAllString(string(cf), "kubernetesVersion: "+vDefaultMM+".0")) // TODO: let <PATCH> version to be the latest one instead of "0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify this TODO? I think it may be removable.

It's intentional that the tests are using .0 to avoid unnecessary testdata updates for every minor release.

Copy link
Contributor Author

@prezha prezha Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

i thought it could be needed sometimes later, but if we would stick always to the .0 patch release, then yes, that TODO is not needed
then also, do we need the testData update section here at all if it is static (ie, the version is always <major>.<minor>.0.) or shall we remove it altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tstromberg i've submitted a new pr where i removed the 'todo' and left testData update section as-is until we clarify it

tstromberg
tstromberg suggested changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

PR looks good and about ready to merge.

hack/kubernetes_version/update_kubernetes_version.go Outdated Show resolved Hide resolved
hack/kubernetes_version/update_kubernetes_version.go Outdated Show resolved Hide resolved
@prezha
update_kubernetes_version: refactor
1444ce3 
- add context.WithTimeout - limit the time spent on quering GitHub
  APIs
- extract Kubernetes GitHub Releases API quering into a separate
  fetchKubernetesReleases func
- extract Kubernetes Version Update logic into a separate
  updateKubernetesVersions func
- extract file patching instructions into a separate replaceAllString
  func
- patch '../../site/content/en/docs/commands/start.md' => remove the
  need to run 'make generate-docs' before 'make test'
- resolve 'ERROR: logging before flag.Parse'
- consolidate and add more verbose logging
- add more descriptive code comments
- removed TODO - will stick to the '0' patch version for all testData
  sets
- ensure 'make test' completes w/o any issues
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 22, 2020
medyagh
medyagh approved these changes Sep 23, 2020
View reviewed changes
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

thanks

@medyagh medyagh merged commit a493400 into kubernetes:master Sep 23, 2020
@prezha prezha mentioned this pull request Feb 19, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@medyagh medyagh medyagh approved these changes

@blueelvis blueelvis Awaiting requested review from blueelvis

@tstromberg tstromberg Awaiting requested review from tstromberg

Assignees

@tstromberg tstromberg

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write a script to automatically propose PR's due to a new Kubernetes release
5 participants
@prezha @k8s-ci-robot @minikube-bot @tstromberg @medyagh