Mark --inject-broker flag as deprecated #1661
Conversation
![knative-prow[bot]](https://avatars.githubusercontent.com/in/183726?s=60&v=4){kind=small}
There was a problem hiding this comment.
@dsimansk: 0 warnings.
In response to this:
Description
Per Eventing change in Sugar controller config, user need to enable the behavior.
@rhuss maybe we should considere adding label flag to trigger creation instread of hardcoded name.
Changes
- Mark --inject-broker flag as deprecated
Reference
Fixes #1629
➜ client git:(pr/deprecate-inject-broker) kn trigger create broker-trigger --inject-broker --sink ksvc:broker-display --filter type=dev.knative.sources.ping Flag --inject-broker has been deprecated, effective since 1.4 and will be removed in 1.6 release. Please refer to 'Automatic Broker Creation' section for configuration options, https://knative.dev/docs/eventing/sugar/#automatic-broker-creation.
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.
knative-prow
bot
added
size/L
Codecov Report
@@ Coverage Diff @@
## main #1661 +/- ##
=======================================
Coverage 79.48% 79.49%
=======================================
Files 171 171
Lines 12963 12969 +6
=======================================
+ Hits 10304 10310 +6
Misses 1941 1941
Partials 718 718
Continue to review full report at Codecov.
|
dsimansk
force-pushed
the
pr/deprecate-inject-broker
branch
from
eeb41e4 to
9be2d20
Compare
knative-prow
bot
added
size/XS
| cmd.Flags().BoolVar(&f.InjectBroker, "inject-broker", false, "Create new broker with name default through common annotation") | ||
| cmd.Flags().MarkDeprecated("inject-broker", "effective since 1.4 and will be removed in 1.6 release. \n"+ | ||
| "Please refer to 'Automatic Broker Creation' section for configuration options, "+ | ||
| "https://knative.dev/docs/eventing/sugar/#automatic-broker-creation.") |
There was a problem hiding this comment.
Maybe add the recommendation to create a Broker directly via kn broker create, because that is what a user can do. I think we shouldn't really think about automatic broker creation on this level.
So maybe this a description:
Automatic broker creation is deprecated and might not work anymore for Knative >= 1.4 (depending on server-side configuration). Please manage brokers directly with the `kn broker` commands.
I wouldn't mention that this is deprecated since client 1.4, I think its good enough to know that it's deprecated for the currently used version ;-). Also wouldn't put in links in the help message if not absolutely needed as we can't be sure that this link will stay stable over time.
There was a problem hiding this comment.
Sure, I was thinking about how stable those links might be.
Does it look better? (the prefix is part of cobra MarkDeprecated function)
Flag --inject-broker has been deprecated, automatic broker creation might not work anymore for Knative >= 1.4 (depending on server-side configuration).
Please manage brokers directly with the `kn broker` commands.There was a problem hiding this comment.
looks good, thanks !
/approve
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, 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:
Approvers can indicate their approval by writing |
|
/cherry-pick release-1.4 |
|
@dsimansk: new pull request created: #1665 In response to this:
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. |
Description
Per Eventing change in Sugar controller config, user need to enable the behavior.
@rhuss maybe we should considere adding label flag to trigger creation instread of hardcoded name.
Changes
Reference
Fixes #1629