feat: Add --filename flag to service create command #913

dsimansk · 2020-07-02T14:04:20Z

Description

Allow users to create Services from provided YAML or JSON file.

There're couple of open questions:

Is it necessary to keep NAME parameter for the create command?
- Should the NAME parameter take priority over file provided name?
Should the multi-item (services separated by ---) YAML be supported as well?

Changes

Add new flag --filename to service create command

Reference

Fixes #550

knative-prow-robot

@dsimansk: 1 warning.

In response to this:

Description

Allow users to create Services from provided YAML or JSON file.

There're couple of open questions:

Is it necessary to keep NAME parameter for the create command?

Should the NAME parameter take priority over file provided name?

Should the multi-item (services separated by ---) YAML be supported as well?

Changes

Add new flag --file to service create command

Reference

Fixes #550

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/kn/commands/service/create.go

maximilien · 2020-07-02T16:37:05Z

Is it necessary to keep NAME parameter for the create command?

Yes. I would suggest just to have it as priority. And if --file is passed then ignore it. So the following all work:

kn service create NAME ...
kn service create --file ...

Should the NAME parameter take priority over file provided name?

Yes. If both provided use NAME and/or error

Should the multi-item (services separated by ---) YAML be supported as well

Yes. But I can easily see an argument for supporting one service in first pass and adding this after.

maximilien

Good start. Left some feedback and found some lack of test coverage. One in particular important --- need test for bad input files, which is likely to happen. You may want to also add tests for files for correct structure but inconsistent or bad input values, e.g., invalid name.

Also, some answers to your questions as a comment above.

maximilien · 2020-07-02T16:39:46Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -68,6 +68,8 @@ type ConfigurationEditFlags struct {
 	GenerateRevisionName bool
 	ForceCreate          bool

+	File string


I always debate using Filename instead of File since there is invariably a File object created somewhere later. Of course, File is shorter as a flag name so makes sense too... Argh.

Following the kubectl example we could settle on -f, --filename as --force is not using that shorthand.

Changed the name in 9a3c1b9.

test/e2e/service_file_test.go

pkg/kn/commands/service/create.go

maximilien · 2020-07-02T16:51:12Z

pkg/kn/commands/service/create.go

+	if err != nil {
+		return nil, err
+	}


No tests for problem generating revision as I can comment these and pass tests. This is less important coverage than the comment above.

navidshaikh

should we consider overriding name and namespace as given on the command line?
should we turn off the client-side revision-name generation for file-mode ?

pkg/kn/commands/service/configuration_edit_flags.go

navidshaikh · 2020-07-03T06:19:44Z

pkg/kn/commands/service/create.go

+		return nil, fmt.Errorf("provided service name '%s' doesn't match name from file '%s'", name, service.Name)
+	}
+	// Set namespace in case it's specified as --namespace
+	service.ObjectMeta.Namespace = namespace


should the namespace also follow check as done above for service name ?

I think for the namespace we should just allow an override from the CLI. A typical use case is that you create your service maybe for a different environment in the file but want to apply it here in a different context (that's different from the "name" which is an identifier that does not change across environments).

navidshaikh · 2020-07-03T06:31:22Z

pkg/kn/commands/service/create.go

+	service.ObjectMeta.Namespace = namespace
+
+	// We need to generate revision to have --force replace working
+	revName, err := servinglib.GenerateRevisionName(editFlags.RevisionName, &service)


I am thinking if the client-side revision name generation could be turned off if using files mode?

➜ client git:(create-from-file) ✗ cat /tmp/hello.yaml apiVersion: serving.knative.dev/v1 kind: Service metadata: name: helloworld-go spec: template: spec: containers: - image: gcr.io/knative-samples/helloworld-go env: - name: TARGET value: "Go Sample v1" ➜ client git:(create-from-file) ✗ kn service create helloworld-go -f /tmp/hello.yaml Creating service 'helloworld-go' in namespace 'default': 0.028s The Route is still working to reflect the latest desired specification. 0.061s Configuration "helloworld-go" is waiting for a Revision to become ready. 0.106s ... 3.751s ... 3.795s Ingress has not yet been reconciled. 3.868s Waiting for load balancer to be ready 4.190s Ready to serve. Service 'helloworld-go' created to latest revision 'helloworld-go-5j5mp' is available at URL: http://helloworld-go.default.example.com ➜ client git:(create-from-file) ✗ kn service create helloworld-go -f /tmp/hello.yaml --force Replacing service 'helloworld-go' in namespace 'default':

When I was testing the flag locally I've experienced the above behaviour. When trying to -recreate the service with --force the update hangs and timeouts eventually. However it's not happening for kn service update flow or imperative kn service create due to the fact that we generate revision name by default. May it a serving bug?

yes could be, but we should be sure what it actually happen when using --force. Could you do a debug session and check where exactly it hangs ?

For opening an issue for serving we would need to recreate the steps via yaml files, I think.

Well, the issue is limited to a corner case when the same source is used twice, in other words to replace the same already created same Service. In that case such an update op never replaces the original Service content, causing no set of event to be produced in the cluster for wait to finish without timing out.
There's only one synthetic event ADDED received, but nothing else as the server-side doesn't update anything at all.
The same behaviour can be reproduced with kubectl replace, meaning that original service isn't actually replaced, is still the same with original timestamp etc., but there's no wait for ready that could be blocking the execution.

That kind of issue isn't present in imperative create flow due to the fact that generated revision name is enough of a change to trigger the update/replace mechanism, therefore desired set of events is received in wait function.

I'd suggest the following:

Implement @rhuss's suggestion from this comment

Mark filename flag with markFlagMakesRevision to enforce revision generation

Adding modification from other flags would always trigger new revision name generation, therefore we'll benefit from it for filename as well (although it might be seen as hack as well)

=> Profit.

For a quick win, I would go for the second option. We then can implement that improvement to pick up other options later. wdyt ?

OK, I'm on board with quick win. :) Anyway it'll better to have improvement PR with proper amount of tests and bake time for options combination. I'll create a new issue for it.

Follow-up enhancement issue #923.

rhuss · 2020-07-03T08:57:29Z

Is it necessary to keep NAME parameter for the create command?

Yes. I would suggest just to have it as priority. And if --file is passed then ignore it. So the following all work:

I would keep the NAME argument and make it non-mandatory when --file is given, too, but I also would never ignore anything that a user provides on the command line as this is not what they expect.

My suggestion:

no name arg, name in file --> Take name in file
name arg, no name in file --> Take name arg
name arg and name in file given, both equal --> Take that name
name arg and name in file given, but different --> Error

dsimansk · 2020-07-03T16:10:02Z

@maximilien @rhuss
Based on your suggestions I've refactored the initial name param restrictions in commit d1c8059.

Please note that I've renamed initial flag name to -f, --filename per Michael's suggestion and also to match kubectl naming.

Now it's possible to use with extended conditions suggested by Roland.

kn service create --filename ...
kn service create NAME --filename ... 

kn service create -f ..
kn service create NAME -f ...

maximilien · 2020-07-07T00:49:41Z

@rhuss will you take a pass? I can do a final review tomorrow after you or if you don’t? I like your suggestion.

rhuss · 2020-07-07T08:10:29Z

pkg/kn/commands/service/create.go

+		return nil, err
+	}
+	service.Spec.Template.Name = revName
+


Where does the override from all other options from the CLI happen ? I mean when someone uses

kn service create -f mysvc.yml --service-account mysa

then mysa should be set as service account in the generated service. For array/map like options that's a bit more tricky as we have to decided whether they override those or just work on the ones from the file like with an update (i would opt for the latter). I.e. I would think adding options in addition to the file would act like an update as if the service in the file has been already present on the cluster.

wdyt ?

As this looks a bit more involved I would be happy to add this in a second PR.

Initially I've left it out, because of potential clashes. However from my limited tests so far it looks well good. E.g. env vars are merged, service account added etc. I'll add a few tests to cover those use cases and update the PR. Plus there's another benefit outlined here.

rhuss

Looks good.

I suggest to merge this PR but work on a followup PR to address the part that allows updating arbitrary fields in the service from the YAML file with options from the command line (see comment below).

knative-metrics-robot · 2020-07-08T13:00:22Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	84.6%	84.6%	0.1
pkg/kn/commands/service/create.go	77.8%	81.9%	4.1

maximilien · 2020-07-09T19:10:11Z

/hold
/lgtm

@rhuss feel free to remove hold when ready

maximilien · 2020-07-10T20:50:20Z

/unhold

rhuss · 2020-07-11T08:36:02Z

/retest

rhuss

thanks! looks good to me, but let's continue on this story by allowing the override from CLI options.

This will give us the value add over plain kubectl usage.

/lgtm

knative-prow-robot · 2020-07-11T08:37:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

feat: Add --file flag to service create cmd

0218ea0

mattmoor reviewed Jul 2, 2020

View reviewed changes

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 2, 2020

knative-prow-robot requested review from cppforlife and sixolet July 2, 2020 14:04

knative-prow-robot reviewed Jul 2, 2020

View reviewed changes

pkg/kn/commands/service/create.go Outdated Show resolved Hide resolved

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 2, 2020

dsimansk added 2 commits July 2, 2020 16:13

fix: Fix linter warnings

228610a

fix: Remove unecessary changes

bd7ea6b

maximilien suggested changes Jul 2, 2020

View reviewed changes

dsimansk added 2 commits July 2, 2020 21:13

fix: Reflect review feedback

9a3c1b9

fix: Change flag --file to -f,--filename

07c1e81

navidshaikh reviewed Jul 3, 2020

View reviewed changes

dsimansk added 2 commits July 3, 2020 08:59

fix: Reflect review feedback

d10ecd1

fix: Move test cleanup to correct place

e99a0e2

fix: Refactor name param restrictions

d1c8059

dsimansk changed the title ~~feat: Add --file flag to service create command~~ feat: Add --filename flag to service create command Jul 3, 2020

rhuss reviewed Jul 7, 2020

View reviewed changes

rhuss approved these changes Jul 7, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2020

dsimansk mentioned this pull request Jul 8, 2020

Add support to combine service create --filename with other options #923

Closed

fix: Fix flag name string in func

5ace3b2

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2020

knative-prow-robot assigned maximilien Jul 9, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020

rhuss approved these changes Jul 11, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jul 11, 2020

knative-prow-robot merged commit 41e49b9 into knative:master Jul 11, 2020

dsimansk mentioned this pull request Jul 21, 2020

feat: Add support to combine service create --filename with other options #937

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add --filename flag to service create command #913

feat: Add --filename flag to service create command #913

dsimansk commented Jul 2, 2020 •

edited

Loading

knative-prow-robot left a comment

maximilien commented Jul 2, 2020

maximilien left a comment •

edited

Loading

maximilien Jul 2, 2020

dsimansk Jul 2, 2020 •

edited

Loading

dsimansk Jul 3, 2020

maximilien Jul 2, 2020

navidshaikh left a comment

navidshaikh Jul 3, 2020

rhuss Jul 7, 2020

navidshaikh Jul 3, 2020

dsimansk Jul 3, 2020

rhuss Jul 7, 2020

dsimansk Jul 8, 2020 •

edited

Loading

rhuss Jul 8, 2020

dsimansk Jul 8, 2020

dsimansk Jul 8, 2020

rhuss commented Jul 3, 2020

dsimansk commented Jul 3, 2020

maximilien commented Jul 7, 2020

rhuss Jul 7, 2020

dsimansk Jul 8, 2020

rhuss left a comment

knative-metrics-robot commented Jul 8, 2020

maximilien commented Jul 9, 2020

maximilien commented Jul 10, 2020

rhuss commented Jul 11, 2020

rhuss left a comment

knative-prow-robot commented Jul 11, 2020

feat: Add --filename flag to service create command #913

feat: Add --filename flag to service create command #913

Conversation

dsimansk commented Jul 2, 2020 • edited Loading

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

maximilien commented Jul 2, 2020

maximilien left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh left a 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

dsimansk Jul 8, 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

rhuss commented Jul 3, 2020

dsimansk commented Jul 3, 2020

maximilien commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jul 8, 2020

maximilien commented Jul 9, 2020

maximilien commented Jul 10, 2020

rhuss commented Jul 11, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jul 11, 2020

dsimansk commented Jul 2, 2020 •

edited

Loading

maximilien left a comment •

edited

Loading

dsimansk Jul 2, 2020 •

edited

Loading

dsimansk Jul 8, 2020 •

edited

Loading