OCPBUGS-45295: Make plugins name array items unique #2117
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@jhadvig: This pull request references Jira Issue OCPBUGS-45295, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @jhadvig! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/M
jhadvig
force-pushed
the
OCPBUGS-45295
branch
from
c8b148c to
70c33c8
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jhadvig 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jhadvig: This pull request references Jira Issue OCPBUGS-45295, which is valid. The bug has been moved to the POST state. 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 openshift-eng/jira-lifecycle-plugin repository. |
| // +optional | ||
| Plugins []string `json:"plugins,omitempty"` | ||
| Plugins []PluginName `json:"plugins,omitempty"` |
There was a problem hiding this comment.
When did this API ship?
Is there a finite, enumerable list of plugins, that we as RH control?
What happens if the list is not already completely unique? Have you tested this?
There was a problem hiding this comment.
no its not an enum cause it would be hard to keep track of plugins since they could be provided by 3rd party.
What happens if the list is not already completely unique? Have you tested this?
so console-operator is already taking care of it programatically and makes the set of enabled plugins unique. The issue is mainly in this array, which can cause confusion, when adding plugins with the same name.
| @@ -55,15 +55,22 @@ type ConsoleSpec struct { | |||
| // +optional | |||
| Route ConsoleConfigRoute `json:"route"` | |||
| // plugins defines a list of enabled console plugin names. | |||
| // +listType=atomic | |||
| // +kubebuilder:validation:MaxItems=64 | |||
There was a problem hiding this comment.
Why was the number 64 chosen?
Is it possible for there to be existing instances of this resource with more than 64 items?
| @@ -55,15 +55,22 @@ type ConsoleSpec struct { | |||
| // +optional | |||
| Route ConsoleConfigRoute `json:"route"` | |||
| // plugins defines a list of enabled console plugin names. | |||
There was a problem hiding this comment.
While updating the constraints for this field, it would be good to update the godoc as well to explain in prose what validations exist for this field. Users can't see the markers when using something like oc/kubectl explain so it is helpful to spell it out for them in the godoc.
https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc is a good place to start when thinking about what to include in the godoc
| // ingress allows to configure the alternative ingress for the console. | ||
| // This field is intended for clusters without ingress capability, | ||
| // where access to routes is not possible. | ||
| // +optional | ||
| Ingress Ingress `json:"ingress"` | ||
| } | ||
|
|
||
| // +kubebuilder:validation:MaxLength=128 |
There was a problem hiding this comment.
Why was a max length of 128 chosen?
Are there any plugins we are aware of that might be cutting it close to that 128 character limit?
Where are plugin names derived from?
| // ingress allows to configure the alternative ingress for the console. | ||
| // This field is intended for clusters without ingress capability, | ||
| // where access to routes is not possible. | ||
| // +optional | ||
| Ingress Ingress `json:"ingress"` | ||
| } | ||
|
|
||
| // +kubebuilder:validation:MaxLength=128 | ||
| // +kubebuilder:validation:Pattern=^[a-zA-Z0-9-]+$ |
There was a problem hiding this comment.
A couple things:
- There didn't appear to be any pattern validation on the value previously. Why was this pattern chosen? Would introducing this pattern result in plugins that are no longer able to be used?
- Conventions are shifting away from using the
kubebuilder:validation:Patternmarker in favor of writing a CEL validation, using thekubebuilder:validation:XValidationmarker, to provide users a more clear message when their input is invalid. If this pattern stays, update this to use a CEL validation instead?
| // ingress allows to configure the alternative ingress for the console. | ||
| // This field is intended for clusters without ingress capability, | ||
| // where access to routes is not possible. | ||
| // +optional | ||
| Ingress Ingress `json:"ingress"` | ||
| } | ||
|
|
||
| // +kubebuilder:validation:MaxLength=128 | ||
| // +kubebuilder:validation:Pattern=^[a-zA-Z0-9-]+$ | ||
| type PluginName string |
There was a problem hiding this comment.
This needs godoc with some more information about the type and the validations on it. Good guideline for writing human readable go doc can be found here: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc
There was a problem hiding this comment.
Let's make sure that the detailed documentation ends up on the field itself, rather than the type alias.
The CRD schema will pick up godoc from the field, but sadly not the type alias.
|
PR needs rebase. 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. |
openshift-merge-robot
added
the
needs-rebase
|
@jhadvig: The following tests failed, say
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. |
/assign @JoelSpeed