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

kickstart: pass options to installer #7051

Merged
merged 5 commits into from
Oct 28, 2019
Merged

kickstart: pass options to installer #7051

merged 5 commits into from
Oct 28, 2019

Conversation

oxplot
Copy link
Contributor

@oxplot oxplot commented Oct 10, 2019

  • pass --stable-channel option
  • append --dont-wait option to list of passthrough options
Summary

Passes through kickstarter options to the installer

Component Name

Packaging - Installer

Additional Information

Fixes #7040

- pass --stable-channel option
- append --dont-wait option to list of passthrough options
@squash-labs
Copy link

squash-labs bot commented Oct 10, 2019

Manage this branch in Squash

Test this branch here: https://oxplotpass-stable-channel-opt-ogc2j.squash.io

@netdatabot netdatabot added the area/packaging Packaging and operating systems support label Oct 10, 2019
Copy link
Contributor

@cosmix cosmix left a comment

Choose a reason for hiding this comment

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

Thanks for this. You need to update kickstart-static64.sh too tho, as that was also reported to be failing. Did you check that case too?

@paulkatsoulakis
Copy link
Contributor

we also need the checksum info to be updated on packaging/installer/readme.md when modifying kickstarts

@cakrit
Copy link
Contributor

cakrit commented Oct 10, 2019

Also @paulkatsoulakis please ensure that @knatsakis knows how to update the two scripts, once the PR is merged.

@knatsakis
Copy link
Contributor

knatsakis commented Oct 10, 2019

This looks like a good fix for kickstart.sh. But the case with kickstart-static64.sh is more complicated.

kickstart.sh does generate .environment but this is not the case for kickstart-static64.sh.

In the second case the .environment file is included in netdata-$version.gz.run. So,
.environment has to be fixed in netdata-$version.gz.run in the first place or kickstart-static64.sh must sed it

@oxplot oxplot requested a review from joelhans as a code owner October 10, 2019 16:07
@CLAassistant
Copy link

CLAassistant commented Oct 10, 2019

CLA assistant check
All committers have signed the CLA.

@ilyam8
Copy link
Member

ilyam8 commented Oct 10, 2019

@oxplot please sign the CLA

@@ -194,6 +194,7 @@ while [ -n "${1}" ]; do
shift 1
elif [ "${1}" = "--stable-channel" ]; then
RELEASE_CHANNEL="stable"
inner_opts="${inner_opts} ${1}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @oxplot, are you sure that the inner script accepts the "--stable-channel" option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see — I wrongly assumed based on #7051 (review) that both scripts behave the same.

I suggest then to only fix kickstart and leave kickstart-static64 for another PR, given how trivial this fix is (so it doesn't drag on).

Copy link
Contributor

Choose a reason for hiding this comment

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

If i may say, @knatsakis if we have decided that through netdata-updater.sh for the static install, we want to automatically set the release channel variable in .environment to stable, when the user uses the stable flag so that updater is automatically pulling stable releases, then it makes sense to add the option. But to make it complete we will need to add handling on the other side too (makeself installer).

If we agree that we do not want to automatically force the stable release channel on the static install, as it was the initial decision with @cakrit when we fixed the updater to work for the static install, then indeed no need to add it to inner opts

@paulkatsoulakis
Copy link
Contributor

paulkatsoulakis commented Oct 10, 2019

Also @paulkatsoulakis please ensure that @knatsakis knows how to update the two scripts, once the PR is merged.

Hello, We 've already discussed the details with @knatsakis earlier today, but feel free to ping me should you need more help or documentation

@@ -307,6 +307,7 @@ while [ -n "${1}" ]; do
shift 1
elif [ "${1}" = "--stable-channel" ]; then
RELEASE_CHANNEL="stable"
NETDATA_INSTALLER_OPTIONS="$NETDATA_INSTALLER_OPTIONS --stable-channel"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this so that it doesn't add an extra space when $NETDATA_INSTALLER_OPTIONS is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is harmless in bash and spaces are collapsed. I don't think it justifies the extra fluff. Also, https://github.com/netdata/netdata/blob/master/packaging/installer/kickstart-static64.sh#L193 seems to do the same.

@@ -351,7 +352,7 @@ done

if [ "${INTERACTIVE}" = "0" ]; then
PACKAGES_INSTALLER_OPTIONS="--dont-wait --non-interactive ${PACKAGES_INSTALLER_OPTIONS}"
NETDATA_INSTALLER_OPTIONS="--dont-wait"
NETDATA_INSTALLER_OPTIONS="$NETDATA_INSTALLER_OPTIONS --dont-wait"
Copy link
Contributor

@knatsakis knatsakis Oct 10, 2019

Choose a reason for hiding this comment

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

The same here: could you make this so that it doesn't add an extra space when $NETDATA_INSTALLER_OPTIONS is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my other comment

@oxplot
Copy link
Contributor Author

oxplot commented Oct 11, 2019

are we happy to leave changes to static64 to another PR?

@knatsakis
Copy link
Contributor

are we happy to leave changes to static64 to another PR?

Yes, please leave changes to static64 to another PR.
Kindly undo those changes so that I can merge this PR.

@knatsakis knatsakis self-requested a review October 15, 2019 13:38
@knatsakis knatsakis merged commit e66165b into netdata:master Oct 28, 2019
jackyhuang85 pushed a commit to jackyhuang85/netdata that referenced this pull request Jan 1, 2020
* kickstart: pass options to installer

- pass --stable-channel option
- append --dont-wait option to list of passthrough options

* kickstart: passthrough --stable-channel in static64

* kickstart: update checksums in readme

* Revert "kickstart: passthrough --stable-channel in static64"

This reverts commit 678ebba.

* Revert kickstart-static64 checksum update
Saruspete pushed a commit to Saruspete/netdata that referenced this pull request May 21, 2020
* kickstart: pass options to installer

- pass --stable-channel option
- append --dont-wait option to list of passthrough options

* kickstart: passthrough --stable-channel in static64

* kickstart: update checksums in readme

* Revert "kickstart: passthrough --stable-channel in static64"

This reverts commit 678ebba.

* Revert kickstart-static64 checksum update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs area/packaging Packaging and operating systems support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--stable-channel option not working
8 participants