-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added delivery options to broker create #1670
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vyasgun: 8 warnings.
In response to this:
Description
Added delivery options to
kn broker create
Changes
- Added
--dl-sink
,--timeout
, --backoff-policy
,backoff-delay
,retry-after-max
,--retry
flags- Validation is on the server side for the values
- Added unit test cases
Reference
Fixes #1557
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.
Codecov Report
@@ Coverage Diff @@
## main #1670 +/- ##
==========================================
+ Coverage 79.48% 79.70% +0.21%
==========================================
Files 171 173 +2
Lines 12963 13173 +210
==========================================
+ Hits 10304 10499 +195
- Misses 1941 1951 +10
- Partials 718 723 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments around coverage and help messages, but otherwise looks OK.
6b1b282
to
cf928ea
Compare
cf928ea
to
c2eaef0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a short comment about validation.
There's another angle of this change I haven't noticed at first. Originally broker command groups didn't include update
. There wasn't no options to modify, except from create/delete flow. However, it should be added probably with this addition flags.
@vyasgun wrt my previous comment. IMO we can merge this PR and add broker update afterwards. Let me know what would you prefer. Otherwise looks ready to me. |
@dsimansk Sorry about the delay. I just added the update command to this PR. |
/test all |
/retest |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, vyasgun 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 |
Description
Added delivery options to
kn broker create
Changes
--dl-sink
,--timeout
, --backoff-policy
,backoff-delay
,retry-after-max
,--retry
flagsReference
Fixes #1557