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

Allow version arg to be optional in "kubeadm upgrade apply" #53220

Merged
merged 1 commit into from
Jan 16, 2018
Merged

Allow version arg to be optional in "kubeadm upgrade apply" #53220

merged 1 commit into from
Jan 16, 2018

Conversation

medinatiger
Copy link
Contributor

@medinatiger medinatiger commented Sep 28, 2017 •
edited
Loading

What this PR does / why we need it:

This PR make the version arg optional if --config is specified and .KuberneteVersion is available.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
fixes kubernetes/kubeadm#460

Special notes for your reviewer:

Allow version arg in kubeadm upgrade apply to be optional if config file already have version info

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 28, 2017
@k8s-github-robot k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 28, 2017
@medinatiger
Copy link
Contributor Author

/assign @luxas

@medinatiger medinatiger changed the title Allow version arg in "kubeadm upgrade apply" optional Allow version arg to be optional in "kubeadm upgrade apply" Oct 2, 2017
@medinatiger
Copy link
Contributor Author

/test pull-kubernetes-unit

jamiehannaford
jamiehannaford approved these changes Oct 6, 2017
View reviewed changes
Copy link
Contributor

@jamiehannaford jamiehannaford left a comment

Choose a reason for hiding this comment

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

LGTM. Defer to @luxas though

@timothysc timothysc added this to the v1.8 milestone Oct 11, 2017
@timothysc timothysc added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. area/usability cherrypick-candidate labels Oct 11, 2017
timothysc
timothysc approved these changes Oct 11, 2017
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

I could nit but
/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 Oct 11, 2017
@timothysc
Copy link
Member

Added as cherry-pick candidate, b/c it fixes the UX for a deployed feature.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 13, 2017
@timothysc
Copy link
Member

@medinatiger please add a release note for this PR.

@medinatiger
Copy link
Contributor Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 13, 2017
@jpbetz jpbetz added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed cherrypick-candidate labels Oct 16, 2017
@medinatiger
Copy link
Contributor Author

/retest

@kad
Copy link
Member

kad commented Oct 25, 2017

it will be good if this PR will have release note line, as it changes behaviour in UX

@medinatiger
Copy link
Contributor Author

/release-note "Allow version arg in kubeadm upgrade apply to be optional if config file already have version info"

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 25, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

5 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@luxas
Copy link
Member

luxas commented Nov 23, 2017

/approve cancel
/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2017
@luxas
Copy link
Member

luxas commented Dec 22, 2017

ping @medinatiger

@jamiehannaford
Copy link
Contributor

Hey @medinatiger, do you have any questions about this PR? We really appreciate the effort and would love to get this in.

@medinatiger
Copy link
Contributor Author

@jamiehannaford, thanks for checking. Just back from vacation. I will find some bandwidth this week to address lucas's comment, and get this PR checked in.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 11, 2018
@kubernetes kubernetes deleted a comment from k8s-github-robot Jan 11, 2018
@jamiehannaford
Copy link
Contributor

/test pull-kubernetes-verify

1 similar comment
@medinatiger
Copy link
Contributor Author

/test pull-kubernetes-verify

@medinatiger
Copy link
Contributor Author

@luxas could you please take a look for the modified PR?

luxas
luxas approved these changes Jan 13, 2018
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Two nits only, but now this generally LGTM, thanks @medinatiger!

/approve

I'll let Tim merge this when the nits are fixed

cmd/kubeadm/app/phases/upgrade/configuration.go Outdated
@@ -52,6 +53,29 @@ func FetchConfiguration(client clientset.Interface, w io.Writer, cfgPath string)
return versionedcfg, nil
}

// FetchConfigurationFromFile fetch configuration from a file
func FetchConfigurationFromFile(cfgPath string) (*kubeadmapiext.MasterConfiguration, error) {
fmt.Println("[upgrade/config] FetchConfiguration From File.")
Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't think this func should log anything...

cmd/kubeadm/app/phases/upgrade/configuration.go Outdated
func FetchConfigurationFromFile(cfgPath string) (*kubeadmapiext.MasterConfiguration, error) {
fmt.Println("[upgrade/config] FetchConfiguration From File.")

if cfgPath == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Is this logic necessary or is this case redundant as it's handled inside of ReadFile()?

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 16, 2018 •
edited
Loading

@medinatiger: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-gpu c9d4aa1 link /test pull-kubernetes-e2e-gce-gpu

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.

@medinatiger
Allow version arg in "kubeadm upgrade apply" optional
8362d7f 
It's optional if the config file contain the version information.
@medinatiger
Copy link
Contributor Author

@timothysc I fixed the nit as pointed out by Lucas (I rebase them into a single CL, sorry for the inconvenience). Please merge when you have a chance

luxas
luxas approved these changes Jan 16, 2018
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, medinatiger, timothysc

Associated issue: #460

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 47f8d62 into kubernetes:master Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luxas luxas luxas approved these changes

@jamiehannaford jamiehannaford jamiehannaford approved these changes

@timothysc timothysc timothysc approved these changes

Assignees

@timothysc timothysc

@mikedanese mikedanese

@luxas luxas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/usability cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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. milestone/incomplete-labels release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubeadm upgrade apply should read version from config file
10 participants
@medinatiger @timothysc @kad @k8s-github-robot @fejta-bot @luxas @jamiehannaford @k8s-ci-robot @jpbetz @mikedanese