Revisit sink prefixes: Support only 'ksvc' prefix and drop 'svc', 'service' support for knative service

[navidshaikh]

Fixes #898

• 'ksvc' prefix --> Knative Service (or no prefix means knative service)
• Update all the examples and tests
• Add examples for broker, service and URI in flag help message
• Drop support for 'svc' and 'service' prefixes for knative service, hint in err msg if these prefixes are given

help message:

  -s, --sink string               Addressable sink for events. You can specify broker, knative service or URI. Examples: '--sink broker:nest' for 'nest' broker, '--sink
                                  https://event.receiver.uri' for an URI (identified by 'http' or 'https'), '--sink ksvc:receiver' or '--sink receiver' for 'receiver' knative service.
                                  If prefix is not provided, it is considered as knative service.

[rhuss]

Sounds good, but I'm generally not so happy with the duality of --sink service and --sink svc:service because how would we add a K8s service here ?

A suggestion would be --sink kservice and --sink ksvc:kservice + --sink svc:service for a k8s service. However this would not be backwards compatible :(

[navidshaikh]

However this would not be backwards compatible :(

Yes. Despite supporting prefix for k8s service, I agree that knative service prefix should be called ksvc OR kservice.

How about deprecating 'svc' and 'service' prefixes in favor of 'ksvc' and 'kservice' ?

---

how would we add a K8s service here ?

should we support prefixes for non-knative addressable resources ?

Giving URI or defining sink prefix for non-knative addressable should do.

It should be explicit about GVKs for non-knative addressable than hardcoding GVKs in kn and giving prefix(es) for it ?

[maximilien]

Looks good to me once you address @dsimansk comment. Best.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

[navidshaikh]

/hold

for conclusion at #898

[navidshaikh]

/retest

Error: ReconcileIngressFailed: Ingress reconciliation failed

[navidshaikh]

/unhold

[rhuss]

Looks good but I wonder whether we should keep svc: for one or two release to point to knative service for the sake of backwards compatibility ? I mean we could remove it from all help message, but still evaluate it if present and print a deprecation warning that this will be removed.

Also we should allow overriding svc: with the config (but only this specific prefix because we will free it up later anyway).

Does this make sense ?

[navidshaikh]

This can be done, however I wonder if we should provide this (temporary) UX for allowing to override the default prefix via config values. Our eventing support is experimental, we may choose to put hard stop for default svc prefix and clear the path for k8s service without bringing any confusion/change-of-UX.

[rhuss]

Ok, let's do it that way for now. We will learn anyway if we get some backslash, but otherwise being overcautios is not good as well.

We make Eventing support as experimental, so I think that's fine.

But maybe we can add to error message when a user uses SVC: that this has been moved to ksvc: ?

[navidshaikh]

But maybe we can add to error message when a user uses SVC: that this has been moved to ksvc: ?

@rhuss : sure, PTAL ad8c47e

[rhuss]

I have not tested it but does this also throw an error if a user configures that prefix in its configuration file ? (that should not be the case).

[navidshaikh]

Nope, this check is after key lookup in sinkMapping ( typ, ok := sinkMappings[prefix]) just a couple lines before.

[knative-metrics-robot]

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	57.6%	65.7%	8.1

[rhuss]

Thanks !

/lgtm