PR for using a env file in oc cmdline #10766

ravisantoshgudimetla · 2016-09-01T15:02:13Z

@bparees, As discussed this is PR for #7585. Please provide some feedback on the set of changes made.

bparees · 2016-09-01T17:00:59Z

pkg/cmd/cli/cmd/newapp.go

@@ -164,6 +167,7 @@ func NewCmdNewApplication(commandName string, f *clientcmd.Factory, out io.Write
 	cmd.Flags().StringSliceVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a list of key value pairs (e.g., -p FOO=BAR,BAR=FOO) to set/override parameter values in the template.")
 	cmd.Flags().StringSliceVar(&config.Groups, "group", config.Groups, "Indicate components that should be grouped together as <comp1>+<comp2>.")
 	cmd.Flags().StringSliceVarP(&config.Environment, "env", "e", config.Environment, "Specify key-value pairs of environment variables to set into each container. This doesn't apply to objects created from a template, use parameters instead.")
+	cmd.Flags().StringVar(&config.EnvironmentFile, "env-file", "", "Specify file containing key-value pairs of environment variables to set into each container.")


"Specify a file", and needs the same caveat as --env has about not being applicable to templates.

bparees · 2016-09-01T17:09:53Z

where did the two .py files in this PR come from?

fabianofranz · 2016-09-01T17:14:29Z

@openshift/cli-review

fabianofranz · 2016-09-01T17:17:38Z

@ravisantoshgudimetla thanks for the PR! You need to run hack/update-generated-docs.sh ; hack/update-generated-completions.sh to generate docs and completions.

ravisantoshgudimetla · 2016-09-01T18:10:14Z

I think, I did messed up rebasing my fork, please ignore this, I will update it again with only my changes.

ravisantoshgudimetla · 2016-09-01T19:21:14Z

@fabianofranz , sure, I will generate them once I am done with initial review. Thanks

ravisantoshgudimetla · 2016-09-02T02:44:09Z

pkg/generate/app/cmd/resolve.go

+	// Environment File where the parameters are stored
+	if len(g.EnvironmentFile) != 0 && !app.IsPossibleEnvironmentFile(g.EnvironmentFile) {
+		return nil, fmt.Errorf("when --env-file is specified you must provide a valid env file location")
+	}
 	if len(g.Strategy) != 0 && len(repositories) == 0 && !g.BinaryBuild {
 		return nil, fmt.Errorf("when --strategy is specified you must provide at least one source code location")
 	}


While I agree with your logic, I think it still doesn't explain validation for above lines, especially the logic for checking strategy and repository location should be moved to another function, if I understood you correctly. However, I made the changes the you suggested, now the validation logic resides in newapp.go within validate() function. Also, please note that format for env file is "--env-file=myenvfile" not "--env-file myenvfile"

cobra supports either syntax. the = is optional.

The repository logic belongs here because we are in a section of code that is resolving what type of argument has been supplied. the environment-file argument does not need to be handled in that way.

Oh great, thanks for correcting me. I just tried that without = & it works perfectly fine and for the second part, I get your point now, since we are trying to resolve a component type called repository, we have it here- makes sense.

ravisantoshgudimetla · 2016-09-02T14:09:43Z

@fabianofranz, I have generated the docs and autocompletion.

@bparees , I think I made all the changes suggested by you, except for IsPossibleEnvironmentFile - whether it should be there or not. Please let me know, if you have any other concerns. Once we finalize IsPossibleEnvironmentFile and other concerns, I wish to get started on writing test cases.

bparees · 2016-09-02T14:22:43Z

pkg/generate/app/environmentfile.go

+		fileexists = false
+	}
+	return isFile(value) && fileexists
+}


seems like it would make more sense to just move this logic inside the GetEnvironmentVars function and have GetEnvironmentVars return an error if the path is not an existing file.

bparees · 2016-09-02T17:41:03Z

@ravisantoshgudimetla one last nit and i think you're in good shape to start on tests.

fabianofranz · 2016-09-02T20:07:49Z

pkg/cmd/cli/cmd/newapp.go

@@ -164,6 +167,7 @@ func NewCmdNewApplication(commandName string, f *clientcmd.Factory, out io.Write
 	cmd.Flags().StringSliceVarP(&config.TemplateParameters, "param", "p", config.TemplateParameters, "Specify a list of key value pairs (e.g., -p FOO=BAR,BAR=FOO) to set/override parameter values in the template.")
 	cmd.Flags().StringSliceVar(&config.Groups, "group", config.Groups, "Indicate components that should be grouped together as <comp1>+<comp2>.")
 	cmd.Flags().StringSliceVarP(&config.Environment, "env", "e", config.Environment, "Specify key-value pairs of environment variables to set into each container. This doesn't apply to objects created from a template, use parameters instead.")
+	cmd.Flags().StringVar(&config.EnvironmentFile, "env-file", "", "Specify a file containing key-value pairs of environment variables to set into each container. This doesn't apply to objects created from a template, use parameters instead.")


A few of the other commands that support files also support a directory or the URL to a file. Should we also support it here, or just local path to a single file?

cmd.MarkFlagFilename("env-file") will make command line completions work. You can optionally pass the accepted file extensions but I think that's not the case here.

Also I'm mostly good with the chosen flag name, but here are other options based on some other places where we accept something similar: --env-from-file, --env-from.

if we can get url support for free from some existing tools in openshift/k8s i'm fine with supporting that, otherwise i'm not convinced it's worth it for now.

@bparees I agree. It's in the builder, I don't think it's reusable.

@fabianofranz . Can I append it to cmd.MarkFlagFilename("file", "yaml", "yml", "json") so that it becomes something like cmd.MarkFlagFilename("file", "yaml", "yml", "json", "env-file")

No, just add cmd.MarkFlagFilename("env-file") (the first argument is the flag name) right after the flag declaration. I don't think we should restrict on file extensions once you can have anything here, like a .txt, a filename without extension, etc.

Ok, thanks, will do that.
One more question, do we need that statement, its already not applying any restrictions on the extension of file.

We need that statement for command line completions. Meaning if you do --env-file=[tab][tab] it will suggest files and dirs based on the local path. ;)

bparees · 2016-09-08T18:25:11Z

@ravisantoshgudimetla can you please squash the commits into a single commit and then lgtm.
@fabianofranz anything else?

this will be a post 3.3 merge.

fabianofranz · 2016-09-08T18:29:57Z

LGTM from a cli perspective.

ncdc · 2016-11-22T16:27:33Z

Is this still for 1.4?

bparees · 2016-11-22T16:43:08Z

@ncdc no and unfortunately it appears to be abandoned.

openshift-bot · 2017-01-21T07:37:21Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

bparees · 2017-01-21T15:42:56Z

this was implemented in #12164

bparees assigned php-coder and unassigned php-coder Sep 1, 2016

bparees reviewed Sep 1, 2016
View reviewed changes

ravisantoshgudimetla reviewed Sep 2, 2016
View reviewed changes

danmcp added the priority/P2 label Sep 2, 2016

danmcp assigned fabianofranz Sep 2, 2016

bparees reviewed Sep 2, 2016
View reviewed changes

fabianofranz reviewed Sep 2, 2016
View reviewed changes

bparees added this to the 1.3.1 milestone Sep 8, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016

ravisantoshgudimetla added 2 commits September 16, 2016 18:45

Env file for oc cmd line

06c83a3

Documentation file didn't get added

d030180

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016

smarterclayton modified the milestones: 1.3.1, 1.4.0 Sep 19, 2016

ravisantoshgudimetla mentioned this pull request Sep 20, 2016

Docs for cli #11021

Closed

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 23, 2016

ravisantoshgudimetla added 2 commits October 27, 2016 03:15

Updating fork

20eee52

Fork rebased

7ddb539

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2016

Fork sync

cf73092

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2016

bparees removed this from the 1.4.0 milestone Nov 22, 2016

bparees assigned mmilata Dec 1, 2016

mmilata mentioned this pull request Dec 8, 2016

oc new-app/new-build/process: add --env-file/--param-file parameters #12164

Merged

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2017

bparees closed this Jan 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR for using a env file in oc cmdline #10766

PR for using a env file in oc cmdline #10766

ravisantoshgudimetla commented Sep 1, 2016

bparees Sep 1, 2016

bparees commented Sep 1, 2016

fabianofranz commented Sep 1, 2016

fabianofranz commented Sep 1, 2016

ravisantoshgudimetla commented Sep 1, 2016

ravisantoshgudimetla commented Sep 1, 2016

ravisantoshgudimetla Sep 2, 2016

bparees Sep 2, 2016

ravisantoshgudimetla Sep 2, 2016

ravisantoshgudimetla commented Sep 2, 2016

bparees Sep 2, 2016

bparees commented Sep 2, 2016

fabianofranz Sep 2, 2016

fabianofranz Sep 2, 2016

fabianofranz Sep 2, 2016

bparees Sep 2, 2016

fabianofranz Sep 2, 2016

ravisantoshgudimetla Sep 2, 2016

fabianofranz Sep 2, 2016

ravisantoshgudimetla Sep 2, 2016 •

edited

Loading

fabianofranz Sep 3, 2016

bparees commented Sep 8, 2016

fabianofranz commented Sep 8, 2016

ncdc commented Nov 22, 2016

bparees commented Nov 22, 2016

openshift-bot commented Jan 21, 2017

bparees commented Jan 21, 2017

PR for using a env file in oc cmdline #10766

PR for using a env file in oc cmdline #10766

Conversation

ravisantoshgudimetla commented Sep 1, 2016

Choose a reason for hiding this comment

bparees commented Sep 1, 2016

fabianofranz commented Sep 1, 2016

fabianofranz commented Sep 1, 2016

ravisantoshgudimetla commented Sep 1, 2016

ravisantoshgudimetla commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla commented Sep 2, 2016

Choose a reason for hiding this comment

bparees commented Sep 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 8, 2016

fabianofranz commented Sep 8, 2016

ncdc commented Nov 22, 2016

bparees commented Nov 22, 2016

openshift-bot commented Jan 21, 2017

bparees commented Jan 21, 2017

ravisantoshgudimetla Sep 2, 2016 •

edited

Loading