kn export defect to honor mode #1212

itsmurugappan · 2021-02-03T00:05:21Z

Description

Mode is ignored, when --with-revisions flag is not mentioned, this pr makes sure that mode is always honored.

Changes

"replay" is the default mode
replay without revisions will print ksvc
replay with revisions will print ksvc list
export mode with/without revisions will return knexport type

Reference

Fixes #1087

cc @dsimansk

knative-prow-robot

@itsmurugappan: 0 warnings.

In response to this:

Description

Mode is ignored, when --with-revisions flag is not mentioned, this pr makes sure that mode is always honored.

Changes

"replay" is the default mode

replay without revisions will print ksvc

replay with revisions will print ksvc list

export mode with/without revisions will return knexport type

Reference

Fixes #1087

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 · 2021-02-03T00:56:26Z

/test pull-knative-client-integration-tests

dsimansk · 2021-02-03T11:13:36Z

"replay" is the default mode

I'm leaning towards export mode as default a bit more, since that should be added value of kn service export command. I let the other chime in, not really insisting on it. :)

I'm planing to take a closer look at the code later today to provide feedback.

dsimansk · 2021-02-03T13:36:21Z

pkg/kn/commands/service/export_test.go

-	assert.Error(t, err, "'kn service export --with-revisions' requires a mode, please specify one of replay|export")
-
-	_, err = executeServiceExportCommand(t, tc, "export", tc.latestSvc.ObjectMeta.Name, "--with-revisions", "--mode", "k8s", "-o", "yaml")
-	assert.Error(t, err, "'kn service export --with-revisions' requires a mode, please specify one of replay|export")
 }

 func TestServiceExport(t *testing.T) {


I think we should have a similar set of tests for service export --mode export without --with-revisions that'd also verity that the fix is working as intended and reflecting --mode flag.

added this test

maximilien

/ok-to-test

maximilien · 2021-02-03T19:58:47Z

pkg/kn/commands/service/export_test.go

@@ -56,12 +56,6 @@ func TestServiceExportError(t *testing.T) {

 	_, err := executeServiceExportCommand(t, tc, "export", tc.latestSvc.ObjectMeta.Name)
 	assert.Error(t, err, "'kn service export' requires output format")


Can we have a simple test verifying no latest revision in export?

dsimansk · 2021-02-16T13:38:19Z

Hi @itsmurugappan just a quick status check, any update wrt review comments above?

itsmurugappan · 2021-02-17T02:19:04Z

hi @dsimansk , will make the changes this week. Sorry got side tracked.

dsimansk · 2021-02-22T12:28:12Z

/retest

dsimansk · 2021-02-22T14:14:17Z

/retest

rhuss · 2021-02-24T07:40:20Z

Thanks, lets wait until friday for the poll in #1087 for what to use as the default mode, merge this PR and I think since this mode is marked as experimental still, we can create a 0.21.1 afterwards so that we don't have to wait for 0.22 to land.

itsmurugappan · 2021-03-03T14:45:02Z

/retest

knative-metrics-robot · 2021-03-03T15:57:38Z

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	86.8%	87.6%	0.8

itsmurugappan · 2021-03-03T16:20:33Z

/retest

itsmurugappan · 2021-03-03T17:08:40Z

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

dsimansk

/lgtm

knative-prow-robot · 2021-03-04T13:22:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

dsimansk · 2021-03-04T14:16:02Z

Ups, I haven't noticed it's already approved and going to be merged on lgtm.

@itsmurugappan can you do a follow-up with changelog update please?

David meets all criteria for an approver: - [x] Reviewer for at least 3 months - [x] Primary reviewer for at least 10 substantial PRs to the codebase, e.g. * knative#1246 * knative#1194 * knative#738 * knative#832 * knative#1016 * knative#877 * knative#667 * knative#697 * knative#1212 * knative#835 - [x] Reviewed at least 30 PRs to the codebase ([38 assigned PRs](https://github.com/knative/client/pulls?q=type%3Apr+assignee%3Adsimansk+)) - [x] Nominated by a WG lead (rhuss) Congrats David ! Thanks a lot for your awesome work & contributions.

David meets all criteria for an approver: - [x] Reviewer for at least 3 months - [x] Primary reviewer for at least 10 substantial PRs to the codebase, e.g. * #1246 * #1194 * #738 * #832 * #1016 * #877 * #667 * #697 * #1212 * #835 - [x] Reviewed at least 30 PRs to the codebase ([38 assigned PRs](https://github.com/knative/client/pulls?q=type%3Apr+assignee%3Adsimansk+)) - [x] Nominated by a WG lead (rhuss) Congrats David ! Thanks a lot for your awesome work & contributions.

kn export defect to honor mode

1306b06

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 3, 2021

knative-prow-robot requested review from cppforlife and rhuss February 3, 2021 00:05

knative-prow-robot reviewed Feb 3, 2021

View reviewed changes

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 3, 2021

dsimansk reviewed Feb 3, 2021

View reviewed changes

maximilien approved these changes Feb 3, 2021

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 3, 2021

rhuss mentioned this pull request Feb 15, 2021

Export command doesn't honour --mode flag #1087

Closed

rhuss assigned dsimansk Feb 15, 2021

add test for export mode without revisions

d4cdcf2

change default to export

b0bb594

add export as default

615e8f5

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 Mar 3, 2021

change tests for export mode change

f5b1635

dsimansk approved these changes Mar 4, 2021

View reviewed changes

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

knative-prow-robot merged commit 609d174 into knative:master Mar 4, 2021

itsmurugappan mentioned this pull request Mar 4, 2021

add changelog for 1212 #1249

Merged

itsmurugappan deleted the knexport-defect branch March 4, 2021 18:11

rhuss mentioned this pull request Mar 9, 2021

Add David as approver #1255

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kn export defect to honor mode #1212

kn export defect to honor mode #1212

itsmurugappan commented Feb 3, 2021 •

edited

Loading

knative-prow-robot left a comment

itsmurugappan commented Feb 3, 2021

dsimansk commented Feb 3, 2021

dsimansk Feb 3, 2021

itsmurugappan Feb 18, 2021

maximilien left a comment

maximilien Feb 3, 2021

dsimansk commented Feb 16, 2021 •

edited

Loading

itsmurugappan commented Feb 17, 2021

dsimansk commented Feb 22, 2021

dsimansk commented Feb 22, 2021

rhuss commented Feb 24, 2021

itsmurugappan commented Mar 3, 2021

knative-metrics-robot commented Mar 3, 2021

itsmurugappan commented Mar 3, 2021

itsmurugappan commented Mar 3, 2021

dsimansk left a comment

knative-prow-robot commented Mar 4, 2021

dsimansk commented Mar 4, 2021

		@@ -56,12 +56,6 @@ func TestServiceExportError(t *testing.T) {

		_, err := executeServiceExportCommand(t, tc, "export", tc.latestSvc.ObjectMeta.Name)
		assert.Error(t, err, "'kn service export' requires output format")

kn export defect to honor mode #1212

kn export defect to honor mode #1212

Conversation

itsmurugappan commented Feb 3, 2021 • edited Loading

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

itsmurugappan commented Feb 3, 2021

dsimansk commented Feb 3, 2021

dsimansk Feb 3, 2021

Choose a reason for hiding this comment

itsmurugappan Feb 18, 2021

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

maximilien Feb 3, 2021

Choose a reason for hiding this comment

dsimansk commented Feb 16, 2021 • edited Loading

itsmurugappan commented Feb 17, 2021

dsimansk commented Feb 22, 2021

dsimansk commented Feb 22, 2021

rhuss commented Feb 24, 2021

itsmurugappan commented Mar 3, 2021

knative-metrics-robot commented Mar 3, 2021

itsmurugappan commented Mar 3, 2021

itsmurugappan commented Mar 3, 2021

dsimansk left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Mar 4, 2021

dsimansk commented Mar 4, 2021

itsmurugappan commented Feb 3, 2021 •

edited

Loading

dsimansk commented Feb 16, 2021 •

edited

Loading