add "kn service export" (#653) #669

itsmurugappan · 2020-02-17T04:44:32Z

Fixes #653

Proposed Changes

Add new command kn export
Add unit test

knative-prow-robot

In response to this:

Fixes #653

Proposed Changes

Add new command kn export

Add unit test

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/export.go

knative-prow-robot · 2020-02-17T04:44:47Z

Hi @itsmurugappan. 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.

itsmurugappan · 2020-02-17T04:48:45Z

/assign @rhuss

itsmurugappan · 2020-02-17T04:53:06Z

@rhuss I checked out in the serving slack and have made the changes accordingly. I have added 2 flows

kn service export service-name -o yaml - exports latest service
kn service export service-name --history -o yaml - for getting the service list with current active revisions.

Please review.

itsmurugappan · 2020-02-17T04:54:36Z

/hold

pkg/kn/commands/service/export.go

dsimansk · 2020-02-17T15:50:55Z

/ok-to-test

itsmurugappan · 2020-02-17T19:31:24Z

/test pull-knative-client-integration-tests-latest-release

itsmurugappan · 2020-02-17T20:02:58Z

/unhold

rhuss · 2020-02-17T22:08:00Z

thanks a lot ! I will give it a review tomorrow ...

itsmurugappan · 2020-02-18T22:19:27Z

/test pull-knative-client-integration-tests

itsmurugappan · 2020-02-18T22:49:20Z

/test pull-knative-client-integration-tests

maximilien

Thanks for the contribution. I like the idea and this is a cool feature to have.

See my comments. Please address changes.

Thanks.

pkg/kn/commands/service/export.go

pkg/kn/commands/service/export_test.go

maximilien · 2020-02-28T18:22:02Z

Also (obviously) tests are still failing. Please address as well. Thx

knative-metrics-robot · 2020-02-29T03:43:33Z

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/export.go	Do not exist	83.3%
pkg/kn/commands/service/service.go	85.7%	86.4%	0.6

itsmurugappan · 2020-02-29T17:54:26Z

/test pull-knative-client-integration-tests-latest-release

rhuss · 2020-03-03T12:53:15Z

@itsmurugappan I saved some time to work on a review this week. Please stay tuned ;-)

dsimansk · 2020-03-06T09:39:07Z

pkg/kn/commands/service/export.go

+
+	command := &cobra.Command{
+		Use:   "export NAME",
+		Short: "export a service",


Suggested change

Short: "export a service",

Short: "Export a service.",

Could you please change the short description to be consistent with the rest of commands. It's used in generated .md as a sentence.

dsimansk

It looks good. I've added a suggestion about the short name.

I have a objections to current format of tests, the common approach for the rest of cmds is to split unit and mock tests. However, it isn't impacting functionality added and can be addressed later.

Finally, would have time to add E2E tests also? If you prefer as a follow-up issue/PR?

itsmurugappan · 2020-03-06T14:28:58Z

I have a objections to current format of tests, the common approach for the rest of cmds is to split unit and mock tests. However, it isn't impacting functionality added and can be addressed later.

@dsimansk Thanks for reviewing. I will make the changes. Regarding unit tests , all the unit tests were supposed to move to mock based on this issue #358 .
Please let me know if my understanding is wrong.

dsimansk · 2020-03-06T14:50:06Z

@itsmurugappan sorry for the confusion. My perspective was a bit different based on parts that I worked on recently (the old way) and I didn't know about #358 the new way. So your approach is actually right. :)

maximilien

LGTM pending e2e test. Just to make sure that this gets executed on real services. Thx.

itsmurugappan · 2020-03-10T05:47:01Z

LGTM pending e2e test. Just to make sure that this gets executed on real services. Thx.

will add the e2e

rhuss

Thanks a lot for your contribution, highly appreciated ! It's very well done

Some remarks:

I'm not 100% sold on the name --history option (maybe we should call it something like --replay or --with-traffic-targets, to make clear that we are creating kind of a service reply list which needs to be "applied" with kubectl.
I think you missed the annotations, that a user can add e.g. with kn service create --annotation.
We should wipe out the traffic block if we only export a single service. Anything else would not be usable if there is a traffic block added.
Some real e2e tests would be cool. I'm going to open a new issue for this.

My suggestion to proceed is:

We merge the PR right away.
I create a smaller PR with a name suggestion for the --history option now, so that we can decide on this before we release 0.13.0
I create an issue for the E2E tests.

Sounds good ?

pkg/kn/commands/service/export.go

rhuss · 2020-03-10T08:12:10Z

pkg/kn/commands/service/export.go

+	}
+	flags := command.Flags()
+	commands.AddNamespaceFlags(flags, false)
+	flags.BoolP("history", "r", false, "Export all active revisions")


Can we remove the shortcut for now. I think its better to spell out the option full each time to make it explicit. Also, the -r doesn't really play well with --history. I even probably would go for a --revisions to make the option more self-explanatory for what is exported in addition (and no shortcut option) (and we could also introduce --no-revisions for this boolean option, like we do for all boolean options).

rhuss · 2020-03-10T08:16:17Z

pkg/kn/commands/service/export.go

+	exportedSvc := servingv1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   latestSvc.ObjectMeta.Name,
+			Labels: latestSvc.ObjectMeta.Labels,


what's with custom annotations provided by the user ? We might want to filter out automatically added annotations (which we now), but as we allow a kn service create --annotation, too, we should copy over annotations as well.

rhuss · 2020-03-10T08:18:11Z

pkg/kn/commands/service/export.go

+	exportedSvc := servingv1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   latestSvc.ObjectMeta.Name,
+			Labels: latestSvc.ObjectMeta.Labels,


we would need annotations, too. See below.

rhuss · 2020-03-10T08:22:04Z

pkg/kn/commands/service/export.go

+
+	exportedSvc.Spec.Template = servingv1.RevisionTemplateSpec{
+		Spec:       revision.Spec,
+		ObjectMeta: latestSvc.Spec.ConfigurationSpec.Template.ObjectMeta,


Why not from the revision ?

From revision they were lot other values.

From ksvc is see only the below

metadata: creationTimestamp: null name: hello-v3

rhuss · 2020-03-10T08:24:39Z

pkg/kn/commands/service/export.go

+	params = append(params, clientservingv1.WithService(latestSvc.ObjectMeta.Name))
+
+	// Query for list with filters
+	revisionList, err := client.ListRevisions(params...)


You could directly use

revisionList, err := client.ListRevisions(clientservingv1.WithService(latestSvc.ObjectMeta.Name))

which is shorter and supposedly easier to read the intention.

pkg/kn/commands/service/export.go

knative-prow-robot · 2020-03-10T09:18:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: itsmurugappan, maximilien, 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~~ [maximilien,rhuss]

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

rhuss · 2020-03-10T09:30:12Z

/lgtm

itsmurugappan · 2020-03-10T17:32:35Z

Sounds good ?

Sure.

add kn export (knative#653)

391ee2d

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

knative-prow-robot reviewed Feb 17, 2020

View reviewed changes

pkg/kn/commands/service/export.go Show resolved Hide resolved

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 17, 2020

knative-prow-robot requested review from cppforlife and maximilien February 17, 2020 04:45

add kn export (knative#653)

86f865a

knative-prow-robot assigned rhuss Feb 17, 2020

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

dsimansk reviewed Feb 17, 2020

View reviewed changes

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

dsimansk reviewed Feb 17, 2020

View reviewed changes

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

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 Feb 17, 2020

itsmurugappan added 4 commits February 17, 2020 10:33

review comments for knative#669

0cd315b

add kn export command

768ac45

add kn export command

8082f98

add kn export command

edb7cf0

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

add kn export command

43baea4

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2020

maximilien suggested changes Feb 28, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2020

Merge branch 'master' into dev01

de0e4d6

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2020

itsmurugappan added 3 commits February 28, 2020 17:46

review comments for pr knative#669

5854bb3

review comments for pr knative#669

ec9d369

review comments for kn export

d317429

dsimansk reviewed Mar 6, 2020

View reviewed changes

maximilien approved these changes Mar 9, 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 Mar 9, 2020

rhuss mentioned this pull request Mar 10, 2020

Add E2E tests for "kn service export" #721

Closed

rhuss approved these changes Mar 10, 2020

View reviewed changes

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

rhuss changed the title ~~add kn export (#653)~~ add "kn service export" (#653) Mar 10, 2020

knative-prow-robot merged commit 5c794d3 into knative:master Mar 10, 2020

itsmurugappan deleted the dev01 branch May 25, 2020 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add "kn service export" (#653) #669

add "kn service export" (#653) #669

itsmurugappan commented Feb 17, 2020

knative-prow-robot left a comment

knative-prow-robot commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

dsimansk commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020 •

edited

Loading

rhuss commented Feb 17, 2020

itsmurugappan commented Feb 18, 2020

itsmurugappan commented Feb 18, 2020

maximilien left a comment

maximilien commented Feb 28, 2020 •

edited

Loading

knative-metrics-robot commented Feb 29, 2020

itsmurugappan commented Feb 29, 2020

rhuss commented Mar 3, 2020

dsimansk Mar 6, 2020

dsimansk left a comment

itsmurugappan commented Mar 6, 2020

dsimansk commented Mar 6, 2020

maximilien left a comment

itsmurugappan commented Mar 10, 2020

rhuss left a comment

rhuss Mar 10, 2020

rhuss Mar 10, 2020

rhuss Mar 10, 2020

rhuss Mar 10, 2020

itsmurugappan Mar 13, 2020

rhuss Mar 10, 2020

knative-prow-robot commented Mar 10, 2020

rhuss commented Mar 10, 2020

itsmurugappan commented Mar 10, 2020

add "kn service export" (#653) #669

add "kn service export" (#653) #669

Conversation

itsmurugappan commented Feb 17, 2020

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

dsimansk commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020

itsmurugappan commented Feb 17, 2020 • edited Loading

rhuss commented Feb 17, 2020

itsmurugappan commented Feb 18, 2020

itsmurugappan commented Feb 18, 2020

maximilien left a comment

Choose a reason for hiding this comment

maximilien commented Feb 28, 2020 • edited Loading

knative-metrics-robot commented Feb 29, 2020

itsmurugappan commented Feb 29, 2020

rhuss commented Mar 3, 2020

dsimansk Mar 6, 2020

Choose a reason for hiding this comment

dsimansk left a comment

Choose a reason for hiding this comment

itsmurugappan commented Mar 6, 2020

dsimansk commented Mar 6, 2020

maximilien left a comment

Choose a reason for hiding this comment

itsmurugappan commented Mar 10, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss Mar 10, 2020

Choose a reason for hiding this comment

rhuss Mar 10, 2020

Choose a reason for hiding this comment

rhuss Mar 10, 2020

Choose a reason for hiding this comment

rhuss Mar 10, 2020

Choose a reason for hiding this comment

itsmurugappan Mar 13, 2020

Choose a reason for hiding this comment

rhuss Mar 10, 2020

Choose a reason for hiding this comment

knative-prow-robot commented Mar 10, 2020

rhuss commented Mar 10, 2020

itsmurugappan commented Mar 10, 2020

itsmurugappan commented Feb 17, 2020 •

edited

Loading

maximilien commented Feb 28, 2020 •

edited

Loading