Adds --force flag to service create command (service replace) #79

navidshaikh · 2019-04-26T14:23:40Z

Fixes #13

Note the --force flag

Examples:

  # Create a service 'mysvc' using image at dev.local/ns/image:latest
  kn service create mysvc --image dev.local/ns/image:latest

  # Create a service with multiple environment variables
  kn service create mysvc --env KEY1=VALUE1 --env KEY2=VALUE2 --image dev.local/ns/image:latest

  # Replace a service 's1' with image dev.local/ns/image:v2 using --force flag
  kn service create --force s1 --image dev.local/ns/image:v2

  # Replace environment variables of service 's1' using --force flag
  kn service create --force s1 --env KEY1=NEW_VALUE1 --env NEW_KEY2=NEW_VALUE2 --image dev.local/ns/image:v1

  # Reset resources to default ones of a service 's1' using --force flag
  # (earlier configured resource requests and limits will be replaced with default)
  # (earlier configured environment variables will be cleared too if any)
  kn service create --force s1 --image dev.local/ns/image:v1

knative-prow-robot · 2019-04-26T14:23:55Z

Hi @navidshaikh. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

matzew · 2019-04-26T15:33:37Z

/ok-to-test

pkg/kn/commands/configuration_edit_flags.go

pkg/kn/commands/service_replace.go

pkg/serving/config_changes.go

rhuss · 2019-05-09T15:13:36Z

pkg/kn/commands/service_create.go

+			if force, err := cmd.Flags().GetBool("force"); err != nil {
+				return err
+			} else if force {
+				client.Services(namespace).Delete(args[0], &v1.DeleteOptions{})


What happens if the service does not exist ? In this case, no delete should be attempted.

We can check that we Get call, here I am not error checking for for Delete though, since error is not handled, it creates the service.
@rhuss : Do you have some tweak in mind for this operation ?

Don't delete the service here at all. Instead, do a GET, if it exists copy the resourceVersion to &service, and issue an update.

Why? This preserves revision history.

Using an update instead of a delete/create would also help with the transactional issue I mentioned (if "delete" works but not "create"), because when the "update" fails you still have the situation as before (e.g. the service running).

The only issue I then have is, that this is not really a create, as I think people would expect a clean history for a create. That would again a plus for a 'replace' command or 'update --set' or something.

For my understanding we should implement:

kn service create --force to create a fresh service without history by deleting any old service of this name. No error if the service already exists.

kn service create --replace for replacing a service, keeping the history, but erroring if the service does not exist.

An alternative for the "replace with keeping history" would be kn service replace or kn update --set.

Sorry for fueling that discussion again, but I really think that 'create afresh by potentially overwriting an existing service' (good for development) and 'replace everything, potentially with default values, but keep the history' (good for getting back to a defined and known state) are looking similar but are subtly different.

kn service create --replace for replacing a service, keeping the history, but erroring if the service does not exist.

@rhuss ; should it really bother for erroring out ? as in, IMO the create verb/command shouldn't block on existence of service, though we could put a proper info on stdout if it existed or not.

rhuss

Looks good to me, except some stylistic nit-picking.

However, one question is open wrt/ to transactional behaviour. What happens when for replace the delete worked but not the create ? Is there a way for a rollback ? Because now, you would just end up with the service deleted which is not what you want when you want even when create/replace of a service fails.

pkg/kn/commands/service_create.go

pkg/kn/commands/configuration_edit_flags.go

pkg/kn/commands/service_create.go

pkg/kn/commands/service_create_test.go

sixolet

Getting close.

pkg/kn/commands/configuration_edit_flags.go

sixolet · 2019-05-09T22:01:57Z

pkg/kn/commands/service_create.go

+			if force, err := cmd.Flags().GetBool("force"); err != nil {
+				return err
+			} else if force {
+				client.Services(namespace).Delete(args[0], &v1.DeleteOptions{})


Don't delete the service here at all. Instead, do a GET, if it exists copy the resourceVersion to &service, and issue an update.

Why? This preserves revision history.

pkg/kn/commands/service_create.go

pkg/kn/commands/service_create_test.go

pkg/kn/commands/service_create.go

cppforlife · 2019-05-10T18:52:59Z

pkg/kn/commands/configuration_edit_flags.go

@@ -29,6 +29,7 @@ type ConfigurationEditFlags struct {
 	Image                      string
 	Env                        []string
 	RequestsFlags, LimitsFlags ResourceFlags
+	ForceCreate                bool


i wonder if at some point it would make sense to have

type ConfigurationCreateFlags struct { ConfigurationEditFlags ForceCreate bool ... }

to better separate create from update at the compile time. wdyt?

+1
@cppforlife : Do you want me to update the same PR? or log an issue and address is post v0.1 ?

@navidshaikh up to you on the timeline. i think current implementation is fine.

Logged the suggestion to be addressed later #110 , thanks @cppforlife

Also rebased the PR to resolve the conflicts.

cppforlife · 2019-05-14T17:42:45Z

/approve
/lgtm

there is potential race between other tools running create, but for now this should be fine since cli will just error.

Fixes knative#13

knative-metrics-robot · 2019-05-15T09:42:08Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/configuration_edit_flags.go	81.0%	81.4%	0.4
pkg/kn/commands/service_create.go	75.0%	66.7%	-8.3

cppforlife · 2019-05-15T21:33:24Z

/lgtm
/approve

knative-prow-robot · 2019-05-15T21:33:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cppforlife, navidshaikh

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~~ [cppforlife]

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

…e#79) * Adds --force flag to service create command / service replace Fixes knative#13 * Keeps the resourceVersion of service with --force flag * Updates the tests * Removes unnecessary comments

knative-prow-robot requested review from cppforlife and sixolet April 26, 2019 14:23

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 26, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 26, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 26, 2019

sixolet suggested changes Apr 26, 2019

View reviewed changes

navidshaikh force-pushed the service-replace branch 3 times, most recently from a041f4a to 0d3e345 Compare May 9, 2019 13:41

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2019

navidshaikh changed the title ~~Add service replace command~~ Adds --force flag to service create command (service replace) May 9, 2019

rhuss reviewed May 9, 2019

View reviewed changes

rhuss requested changes May 9, 2019

View reviewed changes

pkg/kn/commands/service_create.go Outdated Show resolved Hide resolved

pkg/kn/commands/configuration_edit_flags.go Show resolved Hide resolved

pkg/kn/commands/service_create.go Outdated Show resolved Hide resolved

pkg/kn/commands/service_create_test.go Outdated Show resolved Hide resolved

sixolet suggested changes May 9, 2019

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 10, 2019