Add command group for ContainerSource #1016

daisy-ycguo · 2020-09-17T00:34:14Z

Description

Add command groups for container source, including

create container source
delete container source
update container source
list container source
describe container source

Reference

Fixes #809

pkg/kn/flags/podspec_helper.go

maximilien

Thanks for contribution. Obviously you are missing tests so this is just quick review for initial feedback.

One key thing that I feel (and may be wrong) is whether some parts of this PR (e.g., flags parsing for env options and others) is not common to other commands? If it is can we avoid duplication?

Thanks.

pkg/kn/commands/source/container/container.go

pkg/kn/commands/source/container/create.go

pkg/kn/flags/podspec.go

pkg/sources/v1alpha2/container_client.go

daisy-ycguo · 2020-09-18T00:26:38Z

Thanks for contribution. Obviously you are missing tests so this is just quick review for initial feedback.

One key thing that I feel (and may be wrong) is whether some parts of this PR (e.g., flags parsing for env options and others) is not common to other commands? If it is can we avoid duplication?

Thanks.

Some of the flag handling is common with the flag handling of service commands group. It also contains a big refactor. Just in order to ease the review, I will submit another PR to refactor the service commands group.

rhuss · 2020-09-29T10:37:27Z

@daisy-ycguo the PR is really cool, I think we should continue to work on this to get it ready for 0.18

I think the coverage is still a bit low.

@dsimansk could you give this PR a review ? That would be really awesome !

dsimansk · 2020-10-30T14:17:42Z

@rhuss sorry I've missed the first ping that was long time ago. I'll definitely take a look and review the PR.

maximilien

Some comments.

Also, any change for a e2e test? Important since that’s a feature tying multiple parts of the system. OK to do so as a separate PR.

pkg/kn/commands/source/container/container_test.go

pkg/kn/commands/source/container/list.go

pkg/kn/commands/source/container/update_test.go

daisy-ycguo · 2020-11-05T11:41:14Z

Some comments.

Also, any change for a e2e test? Important since that’s a feature tying multiple parts of the system. OK to do so as a separate PR.

Let me try to do e2e in this PR. Thank you for reminder.

dsimansk · 2020-11-06T08:56:41Z

pkg/sources/v1alpha2/container_client.go

+	v1alpha2 "knative.dev/eventing/pkg/apis/sources/v1alpha2"
+	"knative.dev/eventing/pkg/client/clientset/versioned/scheme"
+	clientv1alpha2 "knative.dev/eventing/pkg/client/clientset/versioned/typed/sources/v1alpha2"


I wonder if we can aim for v1 here rather than v1alpha2, since it's available on current Eventing master. Therefore it will be in 0.19 release.

https://github.com/knative/eventing/blob/master/pkg/apis/sources/v1/container_types.go

Good suggestion. I will suggest we upgrade all build-in sources (ApiServerSource, SinkBinding and ContainerSource) from v1alpha2 to v1 in another PR.
I don't see much difference in container sources between v1alpha2 and v1. We need to check the spec for other two build-in sources. Agree ?

I have had a similar question to Navid that he actually replied to that it's fine to use older API as we want to support it until it goes away.

/cc @navidshaikh am I right here? :)

I created an issue #1160. Let's do the upgrade in following PRs. Currently, Kn Client support only 1 version of Eventing sources, that is v1alpha2. We might not want to support v1alpha2 of apiserversource, sinkbinding and pingsource, while another version of container source.

Yeah, we are following the "defensive" approach as described in #462

maximilien

/retest

maximilien

/retest

daisy-ycguo · 2020-11-18T09:12:33Z

@navidshaikh @dsimansk @maximilien
Hello, this PR is finally completed, including the whole container commands and e2e test. Sorry it took such a long time. Please help to review. I will address any comments from you. Thank you.

dsimansk

@daisy-ycguo looks good!
I've added a few comments though, mostly around function's return handling, but those are rather cosmetic ones.

dsimansk · 2020-11-30T14:01:29Z

pkg/kn/commands/source/container/create.go

+			if err == nil {
+				fmt.Fprintf(cmd.OutOrStdout(), "ContainerSource '%s' created in namespace '%s'.\n", args[0], namespace)
+			}
+
+			return err


I'd remove the explicit if err == nil check since it's going to be always true due to the condition above.

Suggested change

if err == nil {

fmt.Fprintf(cmd.OutOrStdout(), "ContainerSource '%s' created in namespace '%s'.\n", args[0], namespace)

}

return err

fmt.Fprintf(cmd.OutOrStdout(), "ContainerSource '%s' created in namespace '%s'.\n", args[0], namespace)

return nil

Good suggestions. Thank you. I changed in the new version.

dsimansk · 2020-11-30T14:05:19Z

pkg/kn/commands/source/container/list.go

+				listFlags.EnsureWithNamespace()
+			}
+
+			err = listFlags.Print(sourceList, cmd.OutOrStdout())


The following would be sufficient here. Just return here and remove the rest of code below.

Suggested change

err = listFlags.Print(sourceList, cmd.OutOrStdout())

return listFlags.Print(sourceList, cmd.OutOrStdout())

Good point. Fixed.

dsimansk · 2020-11-30T14:25:06Z

pkg/kn/commands/source/container/update.go

+			err = srcClient.UpdateContainerSource(b.Build())
+			if err == nil {
+				fmt.Fprintf(cmd.OutOrStdout(), "Container source '%s' updated in namespace '%s'.\n", args[0], namespace)
+			}


This kind of breaks the code flow of code for me. Maybe we could first check if err != nil and display similar message error message like in create op.

Changed in the new version.

dsimansk · 2020-11-30T14:27:04Z

pkg/sources/v1alpha2/container_client.go

+	v1alpha2 "knative.dev/eventing/pkg/apis/sources/v1alpha2"
+	"knative.dev/eventing/pkg/client/clientset/versioned/scheme"
+	clientv1alpha2 "knative.dev/eventing/pkg/client/clientset/versioned/typed/sources/v1alpha2"


I have had a similar question to Navid that he actually replied to that it's fine to use older API as we want to support it until it goes away.

/cc @navidshaikh am I right here? :)

knative-metrics-robot · 2020-12-02T13:16:55Z

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
pkg/kn/commands/source/container/container.go	Do not exist	59.1%
pkg/kn/commands/source/container/create.go	Do not exist	84.4%
pkg/kn/commands/source/container/delete.go	Do not exist	82.4%
pkg/kn/commands/source/container/describe.go	Do not exist	58.6%
pkg/kn/commands/source/container/human_readable_flags.go	Do not exist	52.4%
pkg/kn/commands/source/container/list.go	Do not exist	82.4%
pkg/kn/commands/source/container/update.go	Do not exist	78.9%
pkg/sources/v1alpha2/container_client.go	Do not exist	89.5%
pkg/sources/v1alpha2/container_client_mock.go	Do not exist	90.9%

dsimansk

Thanks @daisy-ycguo looks OK now.
/approve
/lgtm

knative-prow-robot · 2020-12-02T13:55:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, 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

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.

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2020

mattmoor reviewed Sep 17, 2020

View reviewed changes

knative-prow-robot requested review from maximilien and rhuss September 17, 2020 00:34

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 17, 2020

daisy-ycguo changed the title ~~[WIP] Add command groups for creating and deleting of ContainerSource~~ [WIP] Add commands for creating and deleting of ContainerSource Sep 17, 2020

daisy-ycguo changed the title ~~[WIP] Add commands for creating and deleting of ContainerSource~~ [WIP] Add commands for creating and deleting ContainerSource Sep 17, 2020

daisy-ycguo force-pushed the container branch from d35403d to b7e652e Compare September 17, 2020 02:58

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

mattmoor reviewed Sep 17, 2020

View reviewed changes

pkg/kn/flags/podspec_helper.go Outdated Show resolved Hide resolved

maximilien suggested changes Sep 17, 2020

View reviewed changes

daisy-ycguo force-pushed the container branch 2 times, most recently from c1598b6 to 096ae12 Compare September 22, 2020 11:47

daisy-ycguo mentioned this pull request Sep 22, 2020

Add ResolvePodSpec to podspec.go and move the related utilities to podspec_helper #1024

Merged

daisy-ycguo force-pushed the container branch from 096ae12 to 9cd3071 Compare September 22, 2020 15:35

daisy-ycguo changed the title ~~[WIP] Add commands for creating and deleting ContainerSource~~ Add commands for creating and deleting ContainerSource Sep 22, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2020

daisy-ycguo force-pushed the container branch from 9cd3071 to eb4e0f7 Compare September 22, 2020 16:07

daisy-ycguo force-pushed the container branch 2 times, most recently from 0076ce4 to 9239aa6 Compare October 22, 2020 07:44

rhuss assigned dsimansk Oct 27, 2020

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

daisy-ycguo force-pushed the container branch from 9239aa6 to 9676b19 Compare November 3, 2020 09:51

maximilien suggested changes Nov 3, 2020

View reviewed changes

pkg/kn/commands/source/container/container_test.go Outdated Show resolved Hide resolved

pkg/kn/commands/source/container/list.go Outdated Show resolved Hide resolved

pkg/kn/commands/source/container/update_test.go Show resolved Hide resolved

daisy-ycguo changed the title ~~Add commands for creating and deleting ContainerSource~~ Add command group for ContainerSource Nov 4, 2020

dsimansk reviewed Nov 6, 2020

View reviewed changes

maximilien approved these changes Nov 16, 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 Nov 16, 2020

daisy-ycguo force-pushed the container branch 2 times, most recently from 48fd3a6 to 0c910c6 Compare November 17, 2020 14:40

maximilien approved these changes Nov 17, 2020

View reviewed changes

daisy-ycguo force-pushed the container branch 2 times, most recently from 829b924 to 1653c14 Compare November 18, 2020 08:21

daisy-ycguo force-pushed the container branch from 1653c14 to c8050a0 Compare November 18, 2020 09:12

dsimansk requested changes Nov 30, 2020

View reviewed changes

Daisy Guo added 6 commits December 2, 2020 20:59

add container source create and delete

05fe385

add update and list commands

12beefc

add describe command

7f8a073

add e2e test

369fa97

add more tests

9e42b43

changes to address review comments

d5ed58f

daisy-ycguo force-pushed the container branch from c8050a0 to d5ed58f Compare December 2, 2020 13:13

dsimansk approved these changes Dec 2, 2020

View reviewed changes

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

knative-prow-robot merged commit 45162fb into knative:master Dec 2, 2020

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

Add command group for ContainerSource #1016

Add command group for ContainerSource #1016

daisy-ycguo commented Sep 17, 2020 •

edited

Loading

maximilien left a comment

daisy-ycguo commented Sep 18, 2020

rhuss commented Sep 29, 2020

dsimansk commented Oct 30, 2020

maximilien left a comment

daisy-ycguo commented Nov 5, 2020

dsimansk Nov 6, 2020

daisy-ycguo Nov 18, 2020

dsimansk Nov 30, 2020

daisy-ycguo Dec 2, 2020 •

edited

Loading

rhuss Dec 15, 2020

maximilien left a comment

maximilien left a comment

daisy-ycguo commented Nov 18, 2020 •

edited

Loading

dsimansk left a comment

dsimansk Nov 30, 2020

daisy-ycguo Dec 2, 2020

dsimansk Nov 30, 2020 •

edited

Loading

daisy-ycguo Dec 2, 2020

dsimansk Nov 30, 2020

daisy-ycguo Dec 2, 2020

dsimansk Nov 30, 2020

knative-metrics-robot commented Dec 2, 2020

dsimansk left a comment

knative-prow-robot commented Dec 2, 2020

	err = listFlags.Print(sourceList, cmd.OutOrStdout())
	return listFlags.Print(sourceList, cmd.OutOrStdout())

Add command group for ContainerSource #1016

Add command group for ContainerSource #1016

Conversation

daisy-ycguo commented Sep 17, 2020 • edited Loading

Description

Reference

maximilien left a comment

Choose a reason for hiding this comment

daisy-ycguo commented Sep 18, 2020

rhuss commented Sep 29, 2020

dsimansk commented Oct 30, 2020

maximilien left a comment

Choose a reason for hiding this comment

daisy-ycguo commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daisy-ycguo Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

daisy-ycguo commented Nov 18, 2020 • edited Loading

dsimansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk Nov 30, 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

knative-metrics-robot commented Dec 2, 2020

dsimansk left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Dec 2, 2020

daisy-ycguo commented Sep 17, 2020 •

edited

Loading

daisy-ycguo Dec 2, 2020 •

edited

Loading

daisy-ycguo commented Nov 18, 2020 •

edited

Loading

dsimansk Nov 30, 2020 •

edited

Loading