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

Script SRE can use to manage upgrades of clusters. #56

Merged
merged 19 commits into from Jul 19, 2019

Conversation

jewzaam
Copy link
Member

@jewzaam jewzaam commented Jul 17, 2019

See README for how to use.
Note you have to run the script multiple times, so it's a less than ideal. But it's a start..

See README for how to use.
Note you have to run the script multiple times, so it's a less than ideal.  But it's a start..
@jewzaam jewzaam requested a review from mwoodson July 17, 2019 16:06
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 17, 2019
@mwoodson
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019
Copy link
Contributor

@lisa lisa left a comment

Choose a reason for hiding this comment

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

Have some idiomatic Bash suggestions.

scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
Copy link
Member

@cblecker cblecker left a comment

Choose a reason for hiding this comment

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

I started reviewing this, and left some specific comments.. but there are two things throughout the script that should be fixed up:

  • Anywhere a variable is used in a command passed to bash, it should be double quoted to prevent shell globbing
  • Anywhere a variable is called (and especially if it's concatenated with another string) it should be wrapped in curly brackets.

The other thing I see is if you're creating temporary files on the filesystem they should either be created out of tree (in $TMPDIR or something), or they should be gitignored so they don't get committed to the git tree.

scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
jewzaam and others added 8 commits July 18, 2019 11:51
Co-Authored-By: Lisa Seelye <18159+lisa@users.noreply.github.com>
Co-Authored-By: Christoph Blecker <admin@toph.ca>
Co-Authored-By: Christoph Blecker <admin@toph.ca>
Co-Authored-By: Christoph Blecker <admin@toph.ca>
Co-Authored-By: Christoph Blecker <admin@toph.ca>
Co-Authored-By: Christoph Blecker <admin@toph.ca>
Co-Authored-By: Lisa Seelye <18159+lisa@users.noreply.github.com>
Co-Authored-By: Christoph Blecker <admin@toph.ca>
@jewzaam
Copy link
Member Author

jewzaam commented Jul 19, 2019

@cblecker @lisa @mwoodson could I get another look post-changes? thanks

scripts/cluster-upgrade.sh Show resolved Hide resolved
jewzaam and others added 2 commits July 19, 2019 08:31
Co-Authored-By: Lisa Seelye <18159+lisa@users.noreply.github.com>
Copy link
Contributor

@lisa lisa left a comment

Choose a reason for hiding this comment

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

Some changes requested and some comments/questions.

Main overall theme is that $OCP_VERSION_FROM will always be set so checking for it to be set after lines 6-13 is redundant.

scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Show resolved Hide resolved
scripts/cluster-upgrade.sh Show resolved Hide resolved
scripts/cluster-upgrade.sh Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
scripts/cluster-upgrade.sh Outdated Show resolved Hide resolved
jewzaam and others added 2 commits July 19, 2019 08:52
Co-Authored-By: Lisa Seelye <18159+lisa@users.noreply.github.com>
Co-Authored-By: Lisa Seelye <18159+lisa@users.noreply.github.com>
Co-Authored-By: Lisa Seelye <18159+lisa@users.noreply.github.com>
Co-Authored-By: Lisa Seelye <18159+lisa@users.noreply.github.com>
@jewzaam
Copy link
Member Author

jewzaam commented Jul 19, 2019

@lisa note if using -n $var need to use [[ -n $var ]]

@lisa
Copy link
Contributor

lisa commented Jul 19, 2019

@lisa note if using -n $var need to use [[ -n $var ]]

Is [ -z $var ] ok though?

@jewzaam
Copy link
Member Author

jewzaam commented Jul 19, 2019

@lisa note if using -n $var need to use [[ -n $var ]]

Is [ -z $var ] ok though?

No, that might as well be [ true ] though I didn't dig into the why.. just needed a solution to make it work. [ ! -z $var ] works too.

@jewzaam
Copy link
Member Author

jewzaam commented Jul 19, 2019

@lisa note if using -n $var need to use [[ -n $var ]]

Is [ -z $var ] ok though?

No, that might as well be [ true ] though I didn't dig into the why.. just needed a solution to make it work. [ ! -z $var ] works too.

Missed this is asking -z, not -n... I haven't checked. Given there's a working solution I'm in favor of stopping. Not really keen on learning all the nuances of these options in bash tbh.

Copy link
Contributor

@lisa lisa left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2019
@jewzaam jewzaam merged commit 761476b into openshift:master Jul 19, 2019
@jewzaam jewzaam deleted the osd-upgrade-script branch July 19, 2019 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm 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

5 participants