Bug 2100312: monitoring: fix Relabel actions in doc comment from lowercase to PascalCase #1227
Conversation
|
Hello @jan--f! Some important instructions when contributing to openshift/api: For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard
OR
Who should apply these qe/docs/px labels?
|
openshift-ci
bot
added
bugzilla/severity-medium
|
@jan--f: This pull request references Bugzilla bug 1943860, which is invalid:
Comment In response to this:
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/test-infra repository. |
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/valid-bug
|
@jan--f: This pull request references Bugzilla bug 1943860, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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/test-infra repository. |
|
/retitle Bug 2100312: monitoring: include lowercase action in relabel_config validation |
openshift-ci
bot
changed the title
openshift-ci
bot
added
bugzilla/severity-low
|
@jan--f: This pull request references Bugzilla bug 2100312, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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/test-infra repository. |
There was a problem hiding this comment.
Kubernetes APIs use PascalCase for enums. Your controller/operator, whatever is passing this to prometheus should map between the kube value and the prometheus value. I would recommend updating the documentation rather than allowing a mixture of values
I don't think we want to allow both versions here
monitoring/v1alpha1/types.go
Outdated
| @@ -331,8 +331,8 @@ type RelabelConfig struct { | |||
| // action to perform based on regex matching. Must be one of: replace, keep, | |||
| // drop, hashmod, labelmap, labeldrop, or labelkeep. Default is: 'replace' | |||
There was a problem hiding this comment.
This comment should be updated to include the PascalCase versions instead of the lower case versions
There was a problem hiding this comment.
Thanks, I'll fix that.
Just for context, I was using https://github.com/prometheus-operator/prometheus-operator/blob/9bef66a611c01c60faacb705960b98a3fba7dd6d/pkg/apis/monitoring/v1/types.go#L1106 as inspiration.
There was a problem hiding this comment.
Unfortunately we don't control that upstream API. But within OpenShift, we are trying to keep the API consistent no matter the upstream or backend project, hence sticking with just PascalCase
There was a problem hiding this comment.
I added another commit to make the field names in the doc comments actually match the CRD field names. WDYT?
There was a problem hiding this comment.
We actually prefer for the names to match the json tag names, that way when a user does oc explain it shows the json tag name, which is what they will interact with in their YAML file. I would drop the second commit from this PR, but the first commit LGTM I think
There was a problem hiding this comment.
Thanks I'll drop that. Based on a few examples I looked at I wasn't sure what the preferred variant is.
…alCase Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
jan--f
force-pushed
the
alert-relabel-config-actions
branch
2 times, most recently
from
f0b9e35
to
eeaa712
Compare
|
/lgtm |
|
@jan--f: 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/test-infra repository. I understand the commands that are listed here. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, jan--f, JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@jan--f: All pull requests linked via external trackers have merged: Bugzilla bug 2100312 has been moved to the MODIFIED state. In response to this:
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/test-infra repository. |
|
/cherry-pick release-4.11 |
|
@jan--f: new pull request created: #1239 In response to this:
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/test-infra repository. |
jan--f
changed the title
The action in alert relabel configs documents actions with starting with
a lowercase letter but the validation only passes capitalized actions.
The resulting prometheus yaml is always lowercase and controllers call
ToLower anyway. Accepting both lowercase actions and capitalied actions
is consistent with what prometheus-operator accepts.
Signed-off-by: Jan Fajerski jfajersk@redhat.com