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

-v doesn't allow you to set version #665

Merged
merged 3 commits into from
Nov 6, 2020
Merged

-v doesn't allow you to set version #665

merged 3 commits into from
Nov 6, 2020

Conversation

amanda11
Copy link
Contributor

@amanda11 amanda11 commented Oct 9, 2020

The install.sh supports -v and --version to set the version.

Using --version=3.2.0 then it allows you to configure the specify the version to install.

However if you use -v=3.2.0 or -v3.2.0 then it ignores the version specified and uses the default value set to BRANCH. If you use "-v 3.2.0" then it complains that the version -v is not valid.

I believe that you should have been able to use -v=3.2.0 or --version=3.2.0 - and have amended the setup_args so that it will extract the version number out on -v same as --version.

NB. This appears to be legacy behaviour, so should not be a blocker for 3.3 release.

Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

@amanda11 This is a good catch. Can you also update https://github.com/StackStorm/st2-packages/blob/master/scripts/st2bootstrap-deb.sh#L35 and other el7/el8 scripts? There are also the template that generated those script files at https://github.com/StackStorm/st2-packages/blob/master/scripts/st2bootstrap-deb.template.sh#L27. The main script probably just pass --version but we should fix the other scripts just in case. I think you should update the deb/el7/el8 templates and then regenerate the deb/el7/el8 scripts. Thanks!

@amanda11
Copy link
Contributor Author

@amanda11 This is a good catch. Can you also update https://github.com/StackStorm/st2-packages/blob/master/scripts/st2bootstrap-deb.sh#L35 and other el7/el8 scripts? There are also the template that generated those script files at https://github.com/StackStorm/st2-packages/blob/master/scripts/st2bootstrap-deb.template.sh#L27. The main script probably just pass --version but we should fix the other scripts just in case. I think you should update the deb/el7/el8 templates and then regenerate the deb/el7/el8 scripts. Thanks!

Thanks @m4dcoder for the review - I have updated the extra files, ready for your re-review whenever you have time.

Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

LGTM

@amanda11 amanda11 merged commit 5b940ac into master Nov 6, 2020
@amanda11 amanda11 deleted the fix_install_minusv branch November 6, 2020 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants