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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ease upgrade procedure #400

Merged
merged 5 commits into from Apr 13, 2022
Merged

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Mar 15, 2022

After #399, I thought an upgrade command would be useful, too.

@crownedgrouse, here's a shot at it. 馃槃

Assumptions:

  1. wget is available in target system (no longer required, as we're using curl)
  2. LATEST is available in the kerl repository (it would have to be maintained either manually or by some automated mechanism)
  3. we want the default installation to go to /usr/local/bin (this is already mentioned in the README) default is $PWD, otherwise $KERL_APP_INSTALL_DIR (if set)
  • also, shellcheck shows a lot of warnings, even if harmless. I can fix those, in a separate commit, if you want. (and then we could use it - shellcheck - in CI too 馃槃)

This also adds an option to kerl: KERL_APP_INSTALL_DIR so the consumer can decide on where to install kerl from kerl upgrade (a README.md update was in order, for this).

@paulo-ferraz-oliveira
Copy link
Contributor Author

The goal is to have kerl upgrade install to:

  1. (in case it isn't already) /usr/local/bin (e.g. ./kerl is run from the GitHub clone local repo, and kerl is not installed),
  2. (in case it is already) whatever is the current path already, by overwriting it.

@crownedgrouse
Copy link
Contributor

crownedgrouse commented Mar 15, 2022

do not forget to update bash_completion and zsh_completion.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Mar 15, 2022

Oh, yeah. Let me get to those.

The option has no arguments, which made my life easier :)

@crownedgrouse
Copy link
Contributor

crownedgrouse commented Mar 19, 2022

I think there is no need of a LATEST file.
You can get the latest version number with wget --content-disposition https://github.com/kerl/kerl/releases/latest
as --content-disposition use the redirected filename (currently 2.2.4) .
Doing this in a temporary new directory will give you the last version number.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@crownedgrouse, wouldn't that assume that the latest release is always the highest numbered (version-wise) one?

I was thinking about the case where a support version is created for an older kerl version (it's latest, but not "THE latest"), but that might not be what the maintainers intend (?)

Maintaining LATEST gives certainty as a lock, but if the maintainers are Ok, we can do away with that, sure.

@crownedgrouse
Copy link
Contributor

Kerl contains KERL_VERSION. Getting latest version using wget can simply tell if versions are same. If not, upgrade can be done.

@paulo-ferraz-oliveira
Copy link
Contributor Author

I understand, but let's say tomorrow kerl releases patch 2.1.3 after releasing version 2.2.5 (let's assume there was an important security issue to deal with).

Now let's say you have 2.2.4 installed locally. If LATEST (as I expect) is 2.2.5 the upgrade procedure I'm proposing would update to that one. If there is no LATEST (as you propose) we'd be downloading 2.1.3 which is not the latest version, but the latest release, alone.

And this is where it'd get confusing. If maintainers of kerl do only release versions that increment on top of the latest one, sure, your strategy would work.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@jadeallenx, any change this gets looked at soon? Thanks.

kerl Outdated
current_kerl_path="$(which kerl)"
which_status=$?
if [ $which_status != 0 ]; then
install_folder="/usr/local/bin"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be its own environment variable or otherwise settable to a directory from the command line. (For example, I install my kerl at $HOME/bin/kerl)

Copy link
Contributor

Choose a reason for hiding this comment

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

Current kerl is $0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jadeallenx, agreed. If not set, would the current choice (/usr/local/bin) be acceptable as a default? Should we document this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current kerl is $0 ?

I didn't understand this, sorry. $0 is "current script".

Here (in this place in the code), we're covering the option where kerl is not yet "installed" anywhere (eg. local Git repo), so @jadeallenx's suggestion makes sense, but maybe I missed something (?)

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the feature/upgrade branch 2 times, most recently from b857ffa to 99449e4 Compare April 11, 2022 22:30
kerl Outdated
@@ -2124,6 +2130,15 @@ index caa1ce568b..6ebb3d3a25 100644
_END_PATCH
}

upgrade_kerl() {
install_folder=$1
wget -q -O "$install_folder/kerl" $KERL_GIT_BASE/kerl
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use curl everywhere else in the script currently, can we please use it here too instead of adding a new dependency on wget.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure 馃憤

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 2d2912a

Copy link
Collaborator

@jadeallenx jadeallenx left a comment

Choose a reason for hiding this comment

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

Thanks!

@jadeallenx jadeallenx merged commit f4d7039 into kerl:master Apr 13, 2022
@jadeallenx
Copy link
Collaborator

Really appreciate this. Thank you.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Thank you, @jadeallenx.

@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/upgrade branch April 13, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants