-
Notifications
You must be signed in to change notification settings - Fork 114
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
Allow overriding json values before sending #644
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: smarthall The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0624bf7
to
6d981fd
Compare
6d981fd
to
0a393ad
Compare
only one typo, other than that lgtm. Tested on my cluster and it works as expected.
|
fddb753
to
ce9c13e
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.
Just some minor comments. Looks good otherwise!
# Post an internal-only service log message | ||
osdctl servicelog post ${CLUSTER_ID} -i -p "MESSAGE=This is an internal message" --dry-run | ||
|
||
# Post a short external message |
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.
Nitpick, but here under "external message", the command has a lot of "internal message" content for the summary and description. Not a real issue, but might confuse someone
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.
Actually, I'm not quite sure why you can override the -i
(--internal
) here, with -r internal_only=False
. That seems unintuitive. That might be a good use case to say something like "-i is incompatible with -r internal_only=false". Definitely an edge case, though.
Can you elaborate?
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 wanted to avoid adding another command line argument (like -e
) that includes a blank template, or a template with some sane defaults. As we discussed lets come back and add it in the future if required.
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 feel it would be a bit confusing if we have to use -i
to combine with the -r
. I will make me think the override works for internal messages only.
Could it be possible we just can make the template optional if the -r
specified?
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.
Okay I'll make a change to that if neither -t
or -i
is specified, and at least one -r
option is supplied the following template is used by default:
{
"severity": "Info",
"service_name": "SREManualAction",
"internal_only": true
}
This way the user has to explicitly override the summary
and description
. Then if they want to send it to the customer they also have to explicitly override the internal_only
variable.
95cd1e3
to
42fc354
Compare
Combined with the `-i` option this makes it possible to send a servicelog without using a template at all.
42fc354
to
93409e1
Compare
@smarthall: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Combined with the
-i
option this makes it possible to send a servicelog without using a template at all.Solves: https://issues.redhat.com/browse/OSD-26164