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

KEP-26: Implementation #1410

Merged
merged 5 commits into from Mar 17, 2020
Merged

KEP-26: Implementation #1410

merged 5 commits into from Mar 17, 2020

Conversation

porridge
Copy link
Member

@porridge porridge commented Mar 10, 2020

What this PR does / why we need it:

Add a --parameter-file flag.
This is for KEP-26

TODO:

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

some of my comments I'm realizing now are based on existing code (prior to your changes). For existing code:

  1. it doesn't make sense to have params.go in the install package. I'm sure there is history... it is only there because install was impl first... but this should be fix
  2. the error reporting a strings.join version of all the errors is ugly... no good.

Regarding your changes:

  1. we should have params and param file as mutually exclusive
  2. there should be a validation function that will error out if CLI called with both
  3. we should only allow 1 param file
  4. the function for parsing param file should return a list of params... then call the original (albeit modified for data types) function that takes an array of params.
  5. we need more godocs around any function that returns map[string]string when the string value is special

The code is easy to reason about and the tests are great!

@@ -34,6 +34,7 @@ var (
func newInstallCmd(fs afero.Fs) *cobra.Command {
options := install.DefaultOptions
var parameters []string
var parameterFiles []string
Copy link
Member

Choose a reason for hiding this comment

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

parameters above L36 is an array... but parameterFile shouldn't be. lets just allow for 1 file.

@@ -53,6 +54,7 @@ func newInstallCmd(fs afero.Fs) *cobra.Command {

installCmd.Flags().StringVar(&options.InstanceName, "instance", "", "The Instance name. (defaults to Operator name appended with -instance)")
installCmd.Flags().StringArrayVarP(&parameters, "parameter", "p", nil, "The parameter name and value separated by '='")
installCmd.Flags().StringArrayVarP(&parameterFiles, "parameter-file", "P", nil, "YAML file with parameters")
Copy link
Member

Choose a reason for hiding this comment

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

and the desc here indicates 1 file... which is good IMO. also my initial thought was this would be -f for file common to the kube world... but I like P

Copy link
Member Author

Choose a reason for hiding this comment

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

The description explains the semantics of a single flag argument, and I made its wording consistent with the description for --parameter:

  -p, --parameter stringArray        The parameter name and value separated by '='
  -P, --parameter-file stringArray   YAML file with parameters

The stringArray is a hint that multiple such flags can be provided. It's not great UX, but native kubectl seems to be even worse in this area:

      --env=[]: Environment variables to set in the container

Copy link
Contributor

Choose a reason for hiding this comment

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

So we allow multiple --parameter-files to be passed? What are the rules then if both have the same key?

@@ -4,13 +4,54 @@ import (
"errors"
"fmt"
"strings"

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I like this file here (kudoctl/cmd/install/)... we will need it for update and upgrade too

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that param handling became more complex, maybe a new package is warranted? Wdyt about cmd/params/parser.go or smth. along those lines?

func GetParameterMap(raw []string) (map[string]string, error) {
// GetParameterMap takes a slice of parameter strings and a slice of parameter filenames as well as a filesystem,
// parses parameters into a map of keys and values
func GetParameterMap(raw []string, filePaths []string, fs afero.Fs) (map[string]string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

by convention we have fs afero.Fs as 1st param

I initially questioned the string value of map[string]string after reading through function it became clear that this is complex data type but is stringified / wrapped. I'm not sure I like the term "wrapped". what is that suppose to mean?
regardless... This function needs more godoc to explain some of that so someone doesn't needed to read through all the details to understand.

Copy link
Member

Choose a reason for hiding this comment

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

This function is all conflated... which we shouldn't do. I would suggest that we allow params or a parameter file. making it mutually exclusive... this would make it so much easier to reason about... then we should have separate functions. one for params (the original) and one for a param file that parses and passes values into the 1st param function.

This would be better in lots of ways including testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • changed the signature to put fs first
  • explained that values are marshalled in godoc
  • perhaps Marshal would be better than Wrap, but I think changing the name is outside the scope of this PR, I'm merely using this pre-existing function
  • I extracted the two key pieces from the function so that it's clear on first sight what it's doing
  • I don't think it makes sense to pass the values to the original function, since the purpose of the original function is concerned with splitting key and value on the = sign, while the function which reads a parameter value file gets the key and value separately from the YAML parser.

var errs []string
parameters := make(map[string]string)

for _, filePath := range filePaths {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any reason to support multiple files... if we do... we need to fix the CLI help message

case string:
valueType = v1beta1.StringValueType
default:
valueType = v1beta1.StringValueType
Copy link
Member

Choose a reason for hiding this comment

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

this type should never happen and we should use clog to log a warning. verbose 1 or 2? I would be ok with verbose of 0... it shouldn't happen :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It will happen whenever a user provides a value of such type in the parameter value file. However since we do not have parameter type (not even parameter name!) information at this stage, we cannot do much about this other than marshal to string, which should DTRT in most cases anyway.

Note that this problem pre-dates this PR. The user can feed any kind of garbage parameter to the CLI and we will happily stuff it into the Instance.
I raised this on slack and the consensus so far seems to be to do validation in the API server layer. But I think you are well aware of this since you filed #1411

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
@porridge
Copy link
Member Author

porridge commented Mar 12, 2020

As for preventing combined -p and -P or preventing more than one -P, I strongly disagree.

  • This is a very useful thing, if you can quickly iterate by overriding a single value without having to edit the whole file.
  • This was explicitly designed and discussed this way.
  • This is a common pattern in other utilities, see for example --env and --env-file in docker run:
porridge@beczulka:~$ head ?.env
==> x.env <==
bla=filex

==> y.env <==
bla=filey
$ docker run -ti --env-file y.env --env-file x.env --env bla=cmd debian env|grep bla=
bla=cmd
$ docker run -ti --env-file x.env --env bla=cmd debian env|grep bla=
bla=cmd
$ docker run -ti --env-file x.env  debian env|grep bla=
bla=filex
$ docker run -ti --env-file x.env --env-file y.env debian env|grep bla=
bla=filey
$ docker run -ti --env-file y.env --env-file x.env debian env|grep bla=
bla=filex
$ 

PTAL @kensipe

@kensipe
Copy link
Member

kensipe commented Mar 12, 2020

@porridge you have really great logic and examples! However I still disagree :)

The value of multiple env and env-files may be useful in docker because its UX is based on developers where a quick override could be useful.... the kudo experience is geared around operator experience. The idea of a param file stems from 2 needs:

  1. the ability to express non-string types, and
  2. the ability to version a file of params that were used against a kudo operator.

I don't see the added complexity adding value. I will review again setting that aside for now.

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

Reject reason is based we need to not conflate param file mgmt with params mgmt.
There are 3 distinct functions here

  1. process params
  2. process param files (resulting in params)
  3. the merging of params in a predictable way

@zen-dog zen-dog changed the title [KEP-26] Implementation. KEP-26: Implementation Mar 12, 2020
Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

LGTM in general, however, I left a question about multiple parameter files and another around params.go location

@@ -53,6 +54,7 @@ func newInstallCmd(fs afero.Fs) *cobra.Command {

installCmd.Flags().StringVar(&options.InstanceName, "instance", "", "The Instance name. (defaults to Operator name appended with -instance)")
installCmd.Flags().StringArrayVarP(&parameters, "parameter", "p", nil, "The parameter name and value separated by '='")
installCmd.Flags().StringArrayVarP(&parameterFiles, "parameter-file", "P", nil, "YAML file with parameters")
Copy link
Contributor

Choose a reason for hiding this comment

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

So we allow multiple --parameter-files to be passed? What are the rules then if both have the same key?

@@ -4,13 +4,54 @@ import (
"errors"
"fmt"
"strings"

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that param handling became more complex, maybe a new package is warranted? Wdyt about cmd/params/parser.go or smth. along those lines?

@porridge
Copy link
Member Author

@zen-dog

So we allow multiple --parameter-files to be passed?

Correct, see my response to Ken's comment

What are the rules then if both have the same key?

These are documented on the site for users and by the added unit test for developers :-)

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
@porridge
Copy link
Member Author

@kensipe the value from accepting both parameters and parameter values files is consistency with similar mechanism in other systems and programs, that sysadmins do expect. I know this because I happen to come from a sysadmin background. I think saying that UX is targeted at developers and not operators is a stretch, but that was just an example. Here is another from systemd.

I refactored the function to hopefully be less conflated :-) PTAL

@zen-dog I moved to another package, PTAL

@zen-dog
Copy link
Contributor

zen-dog commented Mar 16, 2020

@zen-dog I moved to another package, PTAL

👍 and I see how multiple param-files also make sense.

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

LGTM aside from minor file name nit. PTAL and 🚢 Nice work and thx for your patience!

  __________
< Nice work! >
  ----------
         \   ^__^ 
          \  (oo)\_______
             (__)\       )\/\
                 ||----w |
                 ||     ||

pkg/kudoctl/cmd/params/parse.go Outdated Show resolved Hide resolved
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

/lgtm

Really like the changes! nice work!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kep/26 Parameter value files release/highlight This PR is a highlight for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants