Sample config file generation #43

rockspore · 2020-08-18T23:54:19Z

It works as the following:
./cli samples create -c config.yaml [--out-dir samples] [--native --host target-service-host --name target-service-cluster-name --prefix target-service-prefix --tls tls-directory]

--out-dir defaults to ./samples and the cli overwrites existing files.
Those target service related flags as well as tls are only for --native. Users are responsible for preparing the deployment related files for their target services if it's in Istio.

Open to suggestions.

To fix #34

codecov · 2020-08-18T23:54:25Z

Codecov Report

Merging #43 into master will increase coverage by 0.25%.
The diff coverage is 83.63%.

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   80.26%   80.51%   +0.25%     
==========================================
  Files           7        8       +1     
  Lines        1140     1247     +107     
==========================================
+ Hits          915     1004      +89     
- Misses        118      127       +9     
- Partials      107      116       +9

Impacted Files	Coverage Δ
cmd/samples/samples.go	`83.17% <83.17%> (ø)`
cmd/provision/config.go	`87.93% <100.00%> (ø)`
cmd/root.go	`90.24% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82fbf00...7f2fcf0. Read the comment docs.

theganyo · 2020-08-19T00:23:14Z

This looks very cool, I'll try it out. If we are eliminating the need for users to download the -envoy release package just to get the samples, that would be fantastic.

Thoughts on the flags:

We might as well add short flags as well.
Should we require target name and host instead of defaulting?
Maybe out-dir could just be -o/--output?
It would be safer to default to failing if the target directory, and add a param like -f/--force to overwrite.

Thoughts?

rockspore · 2020-08-19T00:38:44Z

This looks very cool, I'll try it out. If we are eliminating the need for users to download the -envoy release package just to get the samples, that would be fantastic.

Thoughts on the flags:

We might as well add short flags as well.
Should we require target name and host instead of defaulting?
Maybe out-dir could just be -o/--output?
It would be safer to default to failing if the target directory, and add a param like -f/--force to overwrite.

Thoughts?

Short flags sounds good! But since the command is taking advantage of the shared.Resolve() right now, many characters such as -o and -f have been taken. If it makes sense to you, the change I can make is to stop using the Resolve() function and write another one to force the usage of a config yaml. As a result, using flags such as -o/--organization and -e/--environment would not work anymore.

I fully agree on the no-overwrite by default strategy. In fact the program didn't allow overwriting till I started the unit tests and went for the convenient route...Will address it.

rockspore · 2020-08-19T14:57:47Z

If the goal is to eliminate the need for the samples in -envoy repo, we may as well generate a httpbin.yaml file with its content commented out in case someone is just following the tutorial.

theganyo · 2020-08-19T15:30:43Z

Yes, removing the need for Resolve() sounds fine to me.

Re: httpbin, yes... we could do that as well. If a user could do the sample create, apply, and run, without any additional work... that would make it less error-prone. But it only makes sense if they're actually specifying httpbin as their target. I suppose we could make that the default (I know, reversing what I said on making those entries required) and generate the whole thing. Then, if they do specify a target, we don't create it.

…y overwrite by default

rockspore · 2020-08-19T16:53:10Z

-o, -h are global flags, specially -h is taken by cobra for --help. So the shorthands for --out (changed from --output) and --host are not enabled. The rest of what we discussed has been implemented.

rockspore · 2020-08-19T19:39:12Z

Working to bin the template files into templates.go.

theganyo · 2020-08-20T15:48:34Z

cmd/samples/samples.go

+	c.Flags().StringVarP(&s.outDir, "out", "", "./samples", "directory to create config files within")
+	c.Flags().StringVarP(&s.TargetService.Name, "name", "n", "httpbin", "target service name")
+	c.Flags().StringVarP(&s.TargetService.Host, "host", "", "httpbin.org", "target service host")
+	c.Flags().StringVarP(&s.TargetService.Prefix, "prefix", "p", "/", "target service prefix")


I think we should remove path rewrite because of the confusing issues around ordering. Thoughts?

Sounds good to me.

Sorry, but I already removed the prefix rewriting. This flag is taking the path for matching, i.e., for the Envoy router to take the request to the desired upstream cluster. Should we remove that as well? I think it probably should stay.

Gotcha. Hmm. But the only benefit would be to allow other endpoints to not be captured by the matcher, correct? Is that important for sample generation?

That's true. All traffics are meant for the only target in the sample. Let me remove it then.

theganyo · 2020-08-20T15:52:53Z

cmd/samples/samples.go

+	}
+
+	c.Flags().StringVarP(&s.ConfigPath, "config", "c", "", "Path to Apigee Remote Service config file")
+	c.Flags().BoolVarP(&s.isNative, "native", "", false, "generate config for native envoy (otherwise assuming istio)")


I wonder... could we pass a template name instead? So, for example, a user could pass in -t native or -t istio-1.7 and have access to all options?

Makes sense.

theganyo · 2020-08-20T15:57:40Z

cmd/samples/samples.go

+		},
+	}
+
+	c.Flags().StringVarP(&s.ConfigPath, "config", "c", "", "Path to Apigee Remote Service config file")


de-capitalize - all the other descriptions start lowercase

Actually, the config file description in the root.go is identical to this. I am going to change both to lower case.

theganyo · 2020-08-20T16:01:25Z

cmd/samples/samples.go

+			if err != nil {
+				return errors.Wrap(err, "loading config yaml file")
+			}
+			return errors.Wrap(s.createSampleConfigs(printf), "creating sample config files")


We should emit a final message when it finishes successfully. Maybe remind them of the next steps: label the default namespace and run kubectl apply on the directory?

rockspore added 3 commits August 18, 2020 18:42

first commit on sample generation

1bfddb2

added sample config generation

67a8397

description

55f63fc

rockspore requested a review from theganyo August 18, 2020 23:55

rockspore marked this pull request as draft August 19, 2020 00:40

added default httpbin for istio; force the use of config; no director…

237a6be

…y overwrite by default

rockspore marked this pull request as ready for review August 19, 2020 16:55

removed several err checks independent from user input in runtime

13018ed

rockspore marked this pull request as draft August 19, 2020 19:39

rockspore added 2 commits August 19, 2020 20:30

using gobindata to put the templates into templates.go

8e7650e

no more file zipping

5c7512b

rockspore marked this pull request as ready for review August 19, 2020 21:09

theganyo requested changes Aug 20, 2020

View reviewed changes

rockspore marked this pull request as draft August 20, 2020 16:20

addressed the comments on PR-43

7f2fcf0

rockspore marked this pull request as ready for review August 20, 2020 16:45

theganyo approved these changes Aug 20, 2020

View reviewed changes

theganyo merged commit 5bd533d into master Aug 20, 2020

theganyo deleted the issue-34 branch August 20, 2020 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sample config file generation #43

Sample config file generation #43

rockspore commented Aug 18, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

theganyo commented Aug 19, 2020

rockspore commented Aug 19, 2020

rockspore commented Aug 19, 2020

theganyo commented Aug 19, 2020

rockspore commented Aug 19, 2020

rockspore commented Aug 19, 2020

theganyo Aug 20, 2020

rockspore Aug 20, 2020

rockspore Aug 20, 2020 •

edited

Loading

theganyo Aug 20, 2020

rockspore Aug 20, 2020

theganyo Aug 20, 2020

rockspore Aug 20, 2020

theganyo Aug 20, 2020

rockspore Aug 20, 2020

theganyo Aug 20, 2020

rockspore Aug 20, 2020

Sample config file generation #43

Sample config file generation #43

Conversation

rockspore commented Aug 18, 2020

codecov bot commented Aug 18, 2020 • edited Loading

Codecov Report

theganyo commented Aug 19, 2020

rockspore commented Aug 19, 2020

rockspore commented Aug 19, 2020

theganyo commented Aug 19, 2020

rockspore commented Aug 19, 2020

rockspore commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockspore Aug 20, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 18, 2020 •

edited

Loading

rockspore Aug 20, 2020 •

edited

Loading