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

[Feat]: Overhaul handling of options for installer code in the kickstart script. #12630

Closed
Ferroin opened this issue Apr 8, 2022 · 5 comments · Fixed by #12943
Closed

[Feat]: Overhaul handling of options for installer code in the kickstart script. #12630

Ferroin opened this issue Apr 8, 2022 · 5 comments · Fixed by #12943
Assignees
Labels
area/packaging Packaging and operating systems support feature request New features

Comments

@Ferroin
Copy link
Member

Ferroin commented Apr 8, 2022

Problem

Currently, the kickstart script blindly saves all unrecognized options to pass them to either netdata-installer.sh or the static installer code. This has a number of issues:

  1. The resultant behavior is inconsistent. netdata-installer.sh and the static installer code do not support the same set of options, and on many systems neither is ever even invoked. This means that any given option may do nothing on some systems, break the install on others, and work correctly on yet others.
  2. Throwing errors for options we no longer support is more complicated, because we have to keep code for those options essentially forever.
  3. There is no obvious way to clearly specify that a given option should be passed to the installer code (we have an environment variable for this, but it is almost certainly not being used by anyone).
  4. Mistyped options are not actually properly handled, resulting in a potential source of confusion for users.

Description

To remedy this, I would like to add two new options, --local-build-options and --static-install-options, to be used for explicitly passing options to netdata-installer.sh and the static build code respectively, and change the current behavior from warning about unrecognized options to explicitly failing on unrecognized options.

Importance

must have

Value proposition

  1. This would allow users to explicitly pass options to the install scripts for a specific install type, resulting in more predictably consistent behavior.
  2. Mistyped options would throw a proper error instead of being blindly ignored.
  3. Long-term code maintenance would become easier with respect to deprecated/removed options.

Proposed implementation

  • --local-build-options should only be accepted if the --build-only option was specified.
  • --static-install-options should only be accepted if the --static-only option was specified.
  • Both options should take the next argument and append it to the NETDATA_INSTALLER_OPTIONS variable.
  • Unrecognized options should result in the following error message:
    Option "--foo" not recognized. If you meant to pass this option to the installer, please use either "--local-install-options" or "--static-install-options" as appropriate.
    
@Ferroin Ferroin added area/packaging Packaging and operating systems support feature request New features labels Apr 8, 2022
@ReifiedException
Copy link
Contributor

Are there supposed to be multiple options? If so, what delimiter do we use?

@ReifiedException
Copy link
Contributor

Also. Can we require these options ( --local-build-options & --static-install-options) to follow --build-only & -static-only options correspondingly?

@Ferroin
Copy link
Member Author

Ferroin commented Apr 11, 2022

Are there supposed to be multiple options? If so, what delimiter do we use?

My expectation here is to grab the next argument after the --static-install-options or --local-build-options argument and simply append that to the internal options string that would be passed to the installer code. So users could specify a single option, or could specify a string containing a list of options, or could even use the option multiple times to specify multiple single options.

IOW, the following should both work, and be equivalent:

  • --local-build-options --disable-ml --local-build-options --disable-ebpf
  • --local-build-options '--disable-ml --disable-ebpf'

Also. Can we require these options ( --local-build-options & --static-install-options) to follow --build-only & -static-only options correspondingly?

I would prefer not to, as we generally do not enforce option ordering right now even for cases where it might make sense. Based on personal experience, while it does make coding the option parsing code easier, it also make life more difficult for users.

I’ve already got an idea of how to do this without strict ordering and without making the code look too complicated, and was actually kind of planning to pick this up myself today, so we can discuss it on the PR once I’ve got that up later today.

@ReifiedException
Copy link
Contributor

The problem is that while you're parsing the command line parameters you don't know if you should handle those --local-build-options or --static-install-options, because --build-only or --static-only may appear later. This makes the whole thing more complicated.
I also thought about making the sanity check in netdata-installer.sh

@Ferroin
Copy link
Member Author

Ferroin commented Apr 11, 2022

In this case it’s not really all that much more complicated. We’re just storing values for these options, ergo we simply need to store them in a way that we can check after all the options are parsed if they were valid or not. We already do this in a couple of cases as is (for example, handling of install prefixes), and in this case it’s just a couple of extra conditionals after the parsing loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/packaging Packaging and operating systems support feature request New features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants