feat: Subscription CRUD #1013

navidshaikh · 2020-09-15T12:26:05Z

Description

Subscription CRUD operations

Changes

Add subscription create, update, list, describe delete commands
Channel type mapping can be used while creating the subscription to refer different channel types
Set default channel type messaging.knative.dev/v1beta1:Channel, i.e. if no prefix is given to --channel, consider it of Channel type.

/lint

Fixes #955

maximilien

/retest

maximilien

Any chance for an e2e test for this?

docs/cmd/kn_subscription.md

pkg/kn/commands/subscription/flags.go

pkg/kn/commands/subscription/create.go

pkg/kn/commands/subscription/create_test.go

pkg/kn/commands/subscription/delete.go

pkg/kn/commands/subscription/delete_test.go

pkg/kn/commands/subscription/describe.go

pkg/kn/commands/subscription/subscription_test.go

pkg/kn/commands/subscription/update.go

pkg/kn/commands/subscription/update_test.go

pkg/messaging/v1beta1/subscriptions_client.go

pkg/messaging/v1beta1/subscriptions_client_mock.go

pkg/messaging/v1beta1/subscriptions_client.go

navidshaikh · 2020-09-28T14:51:51Z

/retest

 ERROR: timeout waiting for pods to come up
 eventing-controller-56cdd9bf45-cmttb    1/1   Running            0     5m42s
 eventing-webhook-8665cc79c6-sfr9d       0/1   CrashLoopBackOff   6     5m42s

rhuss

Looks good to me and is a straight forward implementation for subscriber CRUD handling.

There are two things I would still like to discuss though:

I know that the options UI mimic 1:1 the fields in the resources, but I wonder whether we should do this here as well. I.e. the target for where events go a called a sink for sources and triggers, but subscriber for subscriptions although they are all of the same types and share the same mappings, that we call "sink mappings". I tend to name the 'subscriber' here also --sink or at least make that option an alias. tbh I have no idea why this difference, but from a UI POV this is more than unfortunate. The same, but to a minor degree, for the difference between a --dead-letter-sink (with sink suffix) and --reply (without sink suffix, but still a sink). Ideally either both or non would have a sink prefix.
Should we settle on the v1 API version of subscription? I would say yes, as I think that for initial support we should use the highest available API version.

docs/cmd/kn_source_apiserver_update.md

docs/cmd/kn_subscription_create.md

pkg/kn/commands/flags/sink.go

pkg/kn/commands/subscription/create.go

pkg/kn/commands/subscription/describe.go

pkg/messaging/v1beta1/subscriptions_client.go

navidshaikh · 2020-09-30T09:39:17Z

This is ready for another round of review, fixed the review comments and here are some notable changes:

Flag names revisited:
- -s, --sink string
- --sink-dead-letter string
- --sink-reply string
Subscription list column name revisited
Aliases:
- subscriptions
- sub
Added shared lib:
knative.dev/client/lib/printing

rhuss

cool, thanks !

/lgtm
/approve

knative-prow-robot · 2020-09-30T10:35:58Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

- Add kn subscription command group and CRUDL sub-commands - create/update uses following flag names for better alignment: - --sink for subscriber - --sink-reply for reply - --sink-dead-letter for dead-letter-sink - Add 'subscriptions' and 'sub' aliases - Introduce shared library `knative.dev/client/lib/printing` to print Sink object in describe output

navidshaikh · 2020-09-30T11:39:11Z

/retest

 ERROR: timeout waiting for pods to come up

navidshaikh · 2020-09-30T12:39:54Z

/retest

rhuss · 2020-09-30T13:36:23Z

/retest

navidshaikh · 2020-09-30T14:11:08Z

/retest

looks like knative/eventing#4165 is flaking the CI job

navidshaikh · 2020-09-30T14:12:34Z

/hold

adding IMC as default channel if no prefix is given
see https://knative.slack.com/archives/CE4MVFVAQ/p1601473090002600

CHANGELOG.adoc

pkg/kn/flags/channel_types.go

i.e. if no prefix is given to `--channel`, consider it of `Channel` type

rhuss

Looks good, some suggestion for a more condensed wording in the help message. I would not mention any concrete build int mappings here but defer that more or less to the channel documentation,

pkg/kn/flags/channel_types.go

knative-metrics-robot · 2020-09-30T17:47:23Z

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/flags/sink.go	65.7%	75.0%	9.3
pkg/kn/commands/subscription/create.go	Do not exist	84.1%
pkg/kn/commands/subscription/delete.go	Do not exist	92.9%
pkg/kn/commands/subscription/describe.go	Do not exist	80.5%
pkg/kn/commands/subscription/flags.go	Do not exist	56.0%
pkg/kn/commands/subscription/list.go	Do not exist	80.0%
pkg/kn/commands/subscription/subscription.go	Do not exist	59.1%
pkg/kn/commands/subscription/update.go	Do not exist	80.0%

rhuss · 2020-09-30T18:33:51Z

/retest

navidshaikh · 2020-10-01T08:31:04Z

/retest

   result_collector.go:75: ERROR: Error: ReconcileIngressFailed: Ingress reconciliation failed

rhuss · 2020-10-01T12:42:37Z

/lgtm

navidshaikh · 2020-10-01T14:46:25Z

/unhold

rhuss · 2020-10-01T16:06:59Z

/retest

* feat: Add subscription CRUD - Add kn subscription command group and CRUDL sub-commands - create/update uses following flag names for better alignment: - --sink for subscriber - --sink-reply for reply - --sink-dead-letter for dead-letter-sink - Add 'subscriptions' and 'sub' aliases - Introduce shared library `knative.dev/client/lib/printing` to print Sink object in describe output * Set default channel type messaging.knative.dev/v1beta1:Channel i.e. if no prefix is given to `--channel`, consider it of `Channel` type * Update e2e tests * Update channel flag description

* Fix channel create example with inbuilt alias for imcv1beta1 (#1005) use `imcv1beta1` alias reference in the example * feat: Subscription CRUD (#1013) * feat: Add subscription CRUD - Add kn subscription command group and CRUDL sub-commands - create/update uses following flag names for better alignment: - --sink for subscriber - --sink-reply for reply - --sink-dead-letter for dead-letter-sink - Add 'subscriptions' and 'sub' aliases - Introduce shared library `knative.dev/client/lib/printing` to print Sink object in describe output * Set default channel type messaging.knative.dev/v1beta1:Channel i.e. if no prefix is given to `--channel`, consider it of `Channel` type * Update e2e tests * Update channel flag description * CHANGELOG for v0.17.1 * Conform with k8s.io/api v0.17.6

* feat: Add subscription CRUD - Add kn subscription command group and CRUDL sub-commands - create/update uses following flag names for better alignment: - --sink for subscriber - --sink-reply for reply - --sink-dead-letter for dead-letter-sink - Add 'subscriptions' and 'sub' aliases - Introduce shared library `knative.dev/client/lib/printing` to print Sink object in describe output * Set default channel type messaging.knative.dev/v1beta1:Channel i.e. if no prefix is given to `--channel`, consider it of `Channel` type * Update e2e tests * Update channel flag description

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 15, 2020

knative-prow-robot requested review from cppforlife and sixolet September 15, 2020 12:26

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

mattmoor reviewed Sep 15, 2020

View reviewed changes

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

mattmoor reviewed Sep 15, 2020

View reviewed changes

maximilien approved these changes Sep 15, 2020

View reviewed changes

maximilien suggested changes Sep 15, 2020

View reviewed changes

docs/cmd/kn_subscription.md Show resolved Hide resolved

pkg/kn/commands/subscription/flags.go Outdated Show resolved Hide resolved

mattmoor reviewed Sep 17, 2020

View reviewed changes

dsimansk reviewed Sep 22, 2020

View reviewed changes

pkg/messaging/v1beta1/subscriptions_client.go Outdated Show resolved Hide resolved

navidshaikh mentioned this pull request Sep 24, 2020

Support Subscription CRUD operations #955

Closed

navidshaikh force-pushed the pr/subscriptions branch from 0db05e6 to 0faca61 Compare September 24, 2020 19:55

navidshaikh changed the title ~~WIP: Subscription CRUD~~ feat: Subscription CRUD Sep 28, 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 28, 2020

rhuss reviewed Sep 28, 2020

View reviewed changes

rhuss reviewed Sep 30, 2020

View reviewed changes

knative-prow-robot assigned rhuss Sep 30, 2020

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

navidshaikh force-pushed the pr/subscriptions branch from dae7284 to 597acf0 Compare September 30, 2020 11:02

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 30, 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 Sep 30, 2020

navidshaikh commented Sep 30, 2020

View reviewed changes

CHANGELOG.adoc Show resolved Hide resolved

navidshaikh commented Sep 30, 2020

View reviewed changes

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

Set default channel type messaging.knative.dev/v1beta1:Channel

f6a6221

i.e. if no prefix is given to `--channel`, consider it of `Channel` type

rhuss reviewed Sep 30, 2020

View reviewed changes

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

navidshaikh added 2 commits September 30, 2020 23:12

Update e2e tests

13fa514

Update channel flag description

1c6da1f

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 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 Oct 1, 2020

knative-prow-robot merged commit ce519b5 into knative:master Oct 1, 2020

navidshaikh deleted the pr/subscriptions branch October 1, 2020 16:58

navidshaikh added the backport/pr A backport PR which is target to a release branch. label Oct 2, 2020

navidshaikh mentioned this pull request Oct 2, 2020

[v0.17.1]: Backport PR for v0.17.1 #1040

Merged

navidshaikh added backport/candidate Consider this PR to be backported to the release branch and removed backport/pr A backport PR which is target to a release branch. labels Oct 2, 2020

navidshaikh added backported-to/0.17 and removed backport/candidate Consider this PR to be backported to the release branch labels Oct 12, 2020

mgencur pushed a commit to openshift-knative/client that referenced this pull request Nov 21, 2022

[release-v1.0.0] Update kn-plugin-func to fix SB template (knative#1013)

252b44b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Subscription CRUD #1013

feat: Subscription CRUD #1013

navidshaikh commented Sep 15, 2020 •

edited

Loading

maximilien left a comment

maximilien left a comment

navidshaikh commented Sep 28, 2020

rhuss left a comment

navidshaikh commented Sep 30, 2020

rhuss left a comment

knative-prow-robot commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

rhuss commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

rhuss left a comment

knative-metrics-robot commented Sep 30, 2020

rhuss commented Sep 30, 2020

navidshaikh commented Oct 1, 2020

rhuss commented Oct 1, 2020

navidshaikh commented Oct 1, 2020

rhuss commented Oct 1, 2020

feat: Subscription CRUD #1013

feat: Subscription CRUD #1013

Conversation

navidshaikh commented Sep 15, 2020 • edited Loading

Description

Changes

maximilien left a comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

navidshaikh commented Sep 28, 2020

rhuss left a comment

Choose a reason for hiding this comment

navidshaikh commented Sep 30, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

rhuss commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

navidshaikh commented Sep 30, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Sep 30, 2020

rhuss commented Sep 30, 2020

navidshaikh commented Oct 1, 2020

rhuss commented Oct 1, 2020

navidshaikh commented Oct 1, 2020

rhuss commented Oct 1, 2020

navidshaikh commented Sep 15, 2020 •

edited

Loading