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.

Sign up for GitHub

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

Jump to bottom

add --ignore-unknown-parameters flag to new-app and process #15230

Merged
merged 1 commit into from
Jul 19, 2017
Merged

add --ignore-unknown-parameters flag to new-app and process #15230

merged 1 commit into from
Jul 19, 2017

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Jul 16, 2017 •
edited by bparees
Loading

'oc process' should be able to ignore invalid parameter names via a flag
fixes #12909

@bparees bparees self-assigned this Jul 17, 2017
@bparees
Copy link
Contributor

bparees commented Jul 17, 2017

[test]

@mfojtik
Copy link
Contributor

mfojtik commented Jul 17, 2017

@bparees don't we have 'required' parameter for this?

@bparees
Copy link
Contributor

bparees commented Jul 17, 2017

@bparees don't we have 'required' parameter for this?

no that's different. required means "you must supply a non-empty string value for this parameter".

this PR is to allow users to do something like:
oc proess -f mytemplate -p some_param_name_that_doesnt_exist=foo --ignore-invalid-parameters

and not get an error.

Your confusion on the subject means the flag name is probably not great since in this case we mean "invalid" as in "does not exist", not "invalid" as in "not a valid value for the parameter".

@matthyx can we try a different flag name? maybe "--skip-absent-parameters"?

@bparees
Copy link
Contributor

bparees commented Jul 17, 2017

[test]

@pweil-
Copy link

pweil- commented Jul 17, 2017

@openshift/cli-review

@matthyx
Copy link
Contributor Author

matthyx commented Jul 17, 2017

@bparees I have the change with the new parameter name... should I push now or I wait until I have tests ready as well?

@bparees
Copy link
Contributor

bparees commented Jul 17, 2017

@matthyx might as well add the tests first.

@fabianofranz
Copy link
Member

Some ideas

--ignore-unknown-parameters
--allow-unknown-parameters
--skip-undeclared-parameters
--skip-param-validation

@bparees
Copy link
Contributor

bparees commented Jul 17, 2017

--ignore-unknown-parameters works for me.

@matthyx
Copy link
Contributor Author

matthyx commented Jul 17, 2017

@bparees done with tests added and squashed

@matthyx matthyx changed the title add --ignore-invalid-parameters flag to new-app and process add --ignore-unknown-parameters flag to new-app and process Jul 17, 2017
bparees
bparees requested changes Jul 17, 2017
View reviewed changes
Copy link
Contributor

@bparees bparees left a comment

Choose a reason for hiding this comment

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

nit on the help text, otherwise lgtm.

pkg/cmd/cli/cmd/newapp.go Outdated
@@ -237,6 +237,7 @@ func NewCmdNewApplication(name, baseName string, f *clientcmd.Factory, in io.Rea
cmd.Flags().StringVar(&config.Name, "name", "", "Set name to use for generated application artifacts")
cmd.Flags().Var(&config.Strategy, "strategy", "Specify the build strategy to use if you don't want to detect (docker|pipeline|source).")
cmd.Flags().StringP("labels", "l", "", "Label to set in all resources for this application.")
cmd.Flags().BoolVar(&config.IgnoreUnknownParameters, "ignore-unknown-parameters", false, "If true, will not stop processing the template when a parameter is not valid.")
Copy link
Contributor

Choose a reason for hiding this comment

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

help text should be "If true, will not stop processing if a provided parameter does not exist in the template."

pkg/cmd/cli/cmd/process.go Outdated
@@ -92,6 +92,7 @@ func NewCmdProcess(fullName string, f *clientcmd.Factory, in io.Reader, out, err
cmd.Flags().StringArrayVarP(params, "param", "p", nil, "Specify a key-value pair (eg. -p FOO=BAR) to set/override a parameter value in the template.")
cmd.Flags().StringArray("param-file", []string{}, "File containing template parameter values to set/override in the template.")
cmd.MarkFlagFilename("param-file")
cmd.Flags().Bool("ignore-unknown-parameters", false, "If true, will not stop processing the template when a parameter is not valid.")
Copy link
Contributor

Choose a reason for hiding this comment

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

same help text change here.

@bparees
Copy link
Contributor

bparees commented Jul 17, 2017

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to e22ac23

@matthyx
Copy link
Contributor Author

matthyx commented Jul 17, 2017 •
edited
Loading

@bparees I did the change and squashed... it seems github would have preferred I did differently :-/

@bparees bparees added this to the 3.7.0 milestone Jul 17, 2017
@bparees
Copy link
Contributor

bparees commented Jul 17, 2017

lgtm, will merge once the 3.6 branch is cut.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3215/) (Base Commit: 6c3acd2) (PR Branch Commit: e22ac23)

@bparees
Copy link
Contributor

bparees commented Jul 19, 2017

[merge][severity:lowrisk]

@bparees
Copy link
Contributor

bparees commented Jul 19, 2017

thanks @matthyx!

@bparees
Copy link
Contributor

bparees commented Jul 19, 2017

flake #15333
flake #13463
flake #14897

[merge]

@bparees
Copy link
Contributor

bparees commented Jul 19, 2017

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 109e55a

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 19, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1314/) (Base Commit: 3bb18b6) (PR Branch Commit: 109e55a) (Extended Tests: lowrisk) (Image: devenv-rhel7_6451)

@openshift-bot
Copy link
Contributor

Origin Action Required: Please contact #openshift-dev to have this pull request manually reviewed and tested

@openshift-bot openshift-bot merged commit d3c6788 into openshift:master Jul 19, 2017
@matthyx matthyx deleted the issue-12909 branch August 19, 2017 12:10
@gunjansharma18
Copy link

In which version is this fixed ? I am using OCP 3.5 and still able to reproduce thos error ?

@bparees
Copy link
Contributor

bparees commented Jul 5, 2018

3.5 is pretty old (3.9 is current and 3.10 is about to release). the fix appears to be in v3.7

@sbose78 sbose78 mentioned this pull request Nov 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparees bparees bparees approved these changes

Assignees

@bparees bparees

Labels
priority/P2
Projects
None yet
Milestone
v3.7
Development

Successfully merging this pull request may close these issues.

'oc process' should be able to ignore invalid parameter names via a flag
7 participants
@matthyx @bparees @mfojtik @pweil- @fabianofranz @openshift-bot @gunjansharma18