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

Exit with error if <version number or publication> is not the final parameter. #37723

Merged
merged 1 commit into from Dec 2, 2016
Merged

Exit with error if <version number or publication> is not the final parameter. #37723

merged 1 commit into from Dec 2, 2016

Conversation

mtaufen
Copy link
Contributor

@mtaufen mtaufen commented Nov 30, 2016 •
edited by pwittrock

getopts stops parsing flags after a non-flag, non-arg-to-a-flag parameter.
This commit adds an error message if any parameters are passed after the
first non-flag, non-arg-to-a-flag parameter in the arg list.

This change is Reviewable

@mtaufen mtaufen added area/upgrade priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 30, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 30, 2016
@mtaufen mtaufen mentioned this pull request Nov 30, 2016
Closed
@mtaufen
Copy link
Contributor Author

mtaufen commented Nov 30, 2016

@saad-ali this will need to be cherry-picked to 1.5

davidopp
davidopp reviewed Nov 30, 2016
View reviewed changes
cluster/gce/upgrade.sh
@@ -33,7 +33,7 @@ source "${KUBE_ROOT}/cluster/kube-util.sh"
function usage() {
echo "!!! EXPERIMENTAL !!!"
echo ""
echo "${0} [-M|-N|-P] -l -o | <version number or publication>"
echo "${0} [-M|-N|-P|-o] -l | <version number or publication>"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be something like
[-M|-N|-P] [-o] [-l]
?

Copy link
Member

Choose a reason for hiding this comment

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

actually
[-M|-N|-P|-l] [-o]
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to [-M|-N|-P] [-o] -l | ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @davidopp. If I were to be pedantic it should be [-M|-N|-P] [-o] (-l | <version number) ;)

@saad-ali
Copy link
Member

@saad-ali this will need to be cherry-picked to 1.5

Ack. Will cherry pick once merged.

@saad-ali saad-ali added this to the v1.5 milestone Nov 30, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2016
@davidopp
Copy link
Member

davidopp commented Nov 30, 2016 •
edited

LGTM

I'm not sure the usage statement is exactly right but I'm not sure how you say "one of this set is required"...

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 30, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 62a098e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit b9384f3. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@eparis
Copy link
Contributor

eparis commented Dec 1, 2016

This appears to be a typical 1.5 PR. Removing the priority/p0 label. Please justify all priority use.

@eparis eparis removed the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 1, 2016
@davidopp davidopp added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 1, 2016
@davidopp
Copy link
Member

davidopp commented Dec 1, 2016

Justification: Node upgrade does not work if user follows the usage that is currently given.

ref/ #37715

@davidopp
Copy link
Member

davidopp commented Dec 1, 2016

Oh sorry, I thought you were asking why it was a 1.5 cherrypick, but you were asking about priority label. I agree about priority label.

@mtaufen
Copy link
Contributor Author

mtaufen commented Dec 1, 2016

-o has an effect if you omit both -N and -M, because in that case it upgrades both master and node. Excellent that we can use (). This makes me happy.

@mtaufen
Copy link
Contributor Author

mtaufen commented Dec 1, 2016

Updated the PR.

@roberthbailey
Copy link
Contributor

"also, can you update the usage for -l to print that it is only supported for upgrading the master and not nodes?"

@mtaufen
Exit with error if <version number or publication> is not the final p…
ee0686b 
…arameter

getopts stops parsing flags after a non-flag, non-arg-to-a-flag parameter.
This commit adds an error message if any parameters are passed after the
first non-flag, non-arg-to-a-flag parameter in the arg list.
@mtaufen
Copy link
Contributor Author

mtaufen commented Dec 2, 2016

Done

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 2, 2016
@davidopp
Copy link
Member

davidopp commented Dec 2, 2016

LGTM

@davidopp davidopp added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Dec 2, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit ee0686b. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit ee0686b. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 6423457 into kubernetes:master Dec 2, 2016
@roberthbailey
Copy link
Contributor

Since the downgrade instructions use upgrade.sh from the 1.4 branch, I think we should cherry pick this into both 1.5 and 1.4. @mtaufen can you create the cherry pick PRs?

@saad-ali saad-ali mentioned this pull request Dec 3, 2016
Closed
@saad-ali saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 3, 2016
@davidopp davidopp mentioned this pull request Dec 3, 2016
Closed
@saad-ali saad-ali mentioned this pull request Dec 3, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Dec 3, 2016
Merge pull request #38038 from saad-ali/automated-cherry-pick-of-#35272-
3787c91 
#37834-#37723-#37668-#37721-#37381-#37944-#37997-#37939-#37990-upstream-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #35272 #37834 #37723 #37668 #37721 #37381 #37944 #37997 #37939 #37990 upstream release 1.5

Batch cherry pick PRs #35272 #37834 #37723 #37668 #37721 #37381 #37944 #37997 #37939 #37990 from master to release-1.5 branch.

PRs #37997 had merge conflicts that needed to be resolved (due to large PRs that merged to master but not 1.5, see this for details)

CC PR Authors: @yarntime @ixdy @mtaufen @ymqytw @derekwaynecarr @jszczepkowski @Kargakis @foxish @jingxu97
@mtaufen
Copy link
Contributor Author

mtaufen commented Dec 10, 2016

@roberthbailey I just saw your message while digging through my pile of notifications. Will do.

@mtaufen mtaufen mentioned this pull request Dec 10, 2016
Merged
@jessfraz jessfraz 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 Dec 10, 2016
jessfraz added a commit that referenced this pull request Dec 10, 2016
@jessfraz
Merge pull request #38539 from mtaufen/automated-cherry-pick-of-#3772…
241f0db 
…3-upstream-release-1.4

Automated cherry pick of #37723
@roberthbailey roberthbailey mentioned this pull request Dec 12, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmarek gmarek gmarek left review comments

@roberthbailey roberthbailey roberthbailey left review comments

@davidopp davidopp davidopp left review comments

Assignees

@roberthbailey roberthbailey

@davidopp davidopp

Labels
area/upgrade 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@mtaufen @saad-ali @k8s-ci-robot @davidopp @eparis @roberthbailey @gmarek @k8s-github-robot @jessfraz