-
Notifications
You must be signed in to change notification settings - Fork 231
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
Update reference docs and book for kpt live #2645
Conversation
a2e60aa
to
987d587
Compare
987d587
to
00946a8
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.
Looks good to me. Few questions.
persistentvolumeclaim/wp-pv-claim created (dry-run) | ||
6 resource(s) applied. 6 created, 0 unchanged, 0 configured, 0 failed (dry-run) | ||
0 resource(s) pruned, 0 skipped, 0 failed (dry-run) | ||
Dry-run strategy: client |
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.
I am thinking if there is a good way to indicate the steps are running in dry-run mode. I could imagine myself mid-way freaking out if I actually applied or it's dry-run and I will have to scroll back to look at the command args.
The previous layout with (dry-run
suffix wasn't terrible. What's the reasoning behind this change ?
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.
There have been some discussions around this. The previous output format is just very verbose, as the output on every line is identical, and in fact only prints the value of the flag provided through the flag. This also aligns the different output formats, as only the events output format had the dry-run suffix.
|
||
--server-side: | ||
Perform the apply operation server-side rather than client-side. | ||
Default value is false (client-side). | ||
|
||
--status-events: |
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.
is it a boolean ? If yes, we should include the default value (true or false)
if it is boolean, we can probably rename it to --enable-status-events
or --{display|show}-status-events
?
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.
I have a PR to change the name to --show-status-events
: #2648.
I have updated the docs with the new name and included the default value.
d69c565
to
ce5a47b
Compare
Update the kpt book and reference docs:
status-events
flag to the reference docsstatus-events
flag is not used.