-
Notifications
You must be signed in to change notification settings - Fork 2k
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
api: Add Notes field to service checks #22397
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.
Hey @nicoche, thanks for the PR! Looks good! I left some minor comments, could you have a look? Also, would you add a changelog file? You can use the make cl
command to generate it.
@@ -68,6 +68,7 @@ type ServiceCheck struct { | |||
Interval time.Duration // Interval of the check | |||
Timeout time.Duration // Timeout of the response from the check before consul fails the check | |||
InitialStatus string // Initial status of the check | |||
Notes string // Specifies arbitrary information for humans. This is not used by Consul internally |
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.
Do you think we could add a little validation on this field? Maybe we could at least limit the length of notes to 250 characters? There's already a method for validating Consul fields.
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.
Good idea, pushed a fix
This reverts commit bf4324cd6f23047c0fbe2fd799235261fa2068bb.
bf4324c
to
7d921be
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.
thanks @nicoche, I just left some minor comments.
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 once @pkazmierczak's changes are addressed.
Co-authored-by: Piotr Kazmierczak <[email protected]>
Thanks guys for checking. @pkazmierczak accepted your suggestions! |
Resolves #21372