-
Notifications
You must be signed in to change notification settings - Fork 208
Update pipectl command to allow specifying labels of event #1419
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
Conversation
|
/lgtm |
| message RegisterEventRequest { | ||
| string name = 1 [(validate.rules).string.min_len = 1]; | ||
| string data = 2 [(validate.rules).string.min_len = 1]; | ||
| map<string,string> labels = 3 [(validate.rules).map.keys.string.min_len = 1, (validate.rules).map.values.string.min_len = 1]; |
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.
This validates just there is no empty value for a key? I just want to know this fails if no labels set. Sorry I'm lazy I didn't read the documentation for the validate.
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.
Yes. No labels case is fine. This checks against the empty string of the key, value.
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.
In case you want to know the generated code
/ Validate checks the field values on RegisterEventRequest with the rules
// defined in the proto definition for this message. If any rules are
// violated, an error is returned.
func (m *RegisterEventRequest) Validate() error {
if m == nil {
return nil
}
if utf8.RuneCountInString(m.GetName()) < 1 {
return RegisterEventRequestValidationError{
field: "Name",
reason: "value length must be at least 1 runes",
}
}
if utf8.RuneCountInString(m.GetData()) < 1 {
return RegisterEventRequestValidationError{
field: "Data",
reason: "value length must be at least 1 runes",
}
}
for key, val := range m.GetLabels() {
_ = val
if utf8.RuneCountInString(key) < 1 {
return RegisterEventRequestValidationError{
field: fmt.Sprintf("Labels[%v]", key),
reason: "value length must be at least 1 runes",
}
}
if utf8.RuneCountInString(val) < 1 {
return RegisterEventRequestValidationError{
field: fmt.Sprintf("Labels[%v]", key),
reason: "value length must be at least 1 runes",
}
}
}
return nil
}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.
That more makes sense! This generated code tell me how it works completely.
| cmd.Flags().StringVar(&r.name, "name", r.name, "The name of event.") | ||
| cmd.Flags().StringVar(&r.data, "data", r.data, "The string value of event data.") | ||
| // TODO: Allow specifying event labels. | ||
| cmd.Flags().StringToStringVar(&r.labels, "labels", r.labels, "The list of labels for event. Format: key=value,key2=value2 .") |
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.
nit: In that case, I feel like the period at the end of a sentence is unneeded.
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 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.
ww. Sure.
|
Thank you! |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1393
Does this PR introduce a user-facing change?: