-
Notifications
You must be signed in to change notification settings - Fork 128
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
chore(cli): improve warnings #1115
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1115 +/- ##
========================================
- Coverage 5.95% 5.91% -0.04%
========================================
Files 31 31
Lines 3126 3144 +18
========================================
Hits 186 186
- Misses 2930 2948 +18
Partials 10 10 ☔ View full report in Codecov by Sentry. |
Based on feedback so far I think we should keep With that in mind, this PR provides inaccurate guidance and shouldn't be merged. |
As discussed, let's make
|
We shouldn’t provide two ways to do the same thing long term. If positional arguments are the future, then the PR is ok, but I’d like @hbagdi to sign off. We chose flags for a reason in the original design |
My feedback is driven to help a user understand the differences in capabilities of the two options, which it seems we have established are staying in the tool ( |
4a3bf34
to
18d2921
Compare
@Tieske Is the CI failing because you tried to remove code coverage from the CI? Can we separate the warnings work from the code coverage change? |
@RobSerafini just flaky CI |
Here is the change that needs to occur with the current deprecation messages for all commands that have been marked deprecated by this change. Currently the messages read as follows:
These will change to:
|
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.
lgtm
@Kong/team-deck can we get this merged in and a release built? tag @teb510 |
@mikefero @mflendrich - I think this just needs an approval and merge and a new build of Deck. Can you guys help move this along? |
Co-authored-by: Gabriele <[email protected]>
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 minor comment. Otherwise, LGTM
Co-authored-by: Gabriele <[email protected]> Co-authored-by: Rick Spurgeon <[email protected]>
2 commits;
EDIT: 2nd commit moved here: Kong/go-database-reconciler#17