Monitoring API: Add AlertmanagerMainConfig #2148
Conversation
|
Hello @marioferh! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: marioferh 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 |
Signed-off-by: Mario Fernandez <[email protected]>
marioferh
force-pushed
the
alertmanager_monitoring_api
branch
from
c0d2965 to
c35227f
Compare
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@marioferh: 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. |
There was a problem hiding this comment.
A lot of the comments are centered around godoc. I'd recommend looking at https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc for more information on what makes a good godoc that is helpful to users.
Another thing that stood out was multiple fields related to pod spec configuration - you may want to group those into a separate struct to have a single field that clearly denotes that the sub-fields in that object map directly to pod spec fields.
| @@ -77,6 +78,9 @@ type ClusterMonitoringSpec struct { | |||
| // userDefined set the deployment mode for user-defined monitoring in addition to the default platform monitoring. | |||
| // +required | |||
| UserDefined UserDefinedMonitoring `json:"userDefined"` | |||
| // alertmanagerMainConfig defines settings for the Alertmanager component in the `openshift-monitoring` namespace. | |||
There was a problem hiding this comment.
While this is a good start in stating what this field does, generally it is recommended to follow the guidelines for writing godoc on the API fields here: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc
In particular, the changes I would suggest here are:
- Making sure there is a clear explanation as to why a user may want to use this field (i.e what does it allow them to achieve?)
- including a reference that the field is required. Users can't see the
+requiredmarker in the generated documentation that is shown when using something likeoc explain ...so being explicit in the godoc ensures that a user will see that information.
| // mode enables or disables the main Alertmanager instance. in the `openshift-monitoring` namespace | ||
| // Allowed values are "Enabled", "Disabled". |
There was a problem hiding this comment.
A few things:
- It would be good to go a bit more in detail on what this actually does when set to a specific value. What actually happens when this is set to
Enabled? - Include a snippet that this field is required, as per my previous comment about tags not being included in generated documentation
- Generally we try to avoid using
EnabledandDisabledas they can be a bit overloaded and similar to True/False. This makes it a bit more difficult to introduce additional modes in the future where it isn't necessarily a clean enabled/disabled. Are there any other names you could use that are more descriptive for the same desired state?
| type AlertmanagerMainConfig struct { | ||
| // mode enables or disables the main Alertmanager instance. in the `openshift-monitoring` namespace | ||
| // Allowed values are "Enabled", "Disabled". | ||
| // +kubebuilder:validation:Enum:=Enabled;Disabled;"" |
There was a problem hiding this comment.
It looks like this validation includes the ability to use an empty string as a value. There is no mention that the empty value is a valid value - is it?
If it is, is this actually a required field? What happens in the case of the empty string value?
| } | ||
|
|
||
| // AlertmanagerMode defines mode for AlertManager instance | ||
| // +kubebuilder:validation:Enum="";Enabled;Disabled |
There was a problem hiding this comment.
This is a repeated enum validation that exists on the AlertmanagerMainConfig.Mode field. This results in two separate enum validations showing up on the generated CRD that may result in unexpected behavior. Maintaining two separate validations may also introduce drift.
Pick one location to add this validation.
|
|
||
| const ( | ||
| // AlertManagerEnable enables the main Alertmanager instance. in the `openshift-monitoring` namespace | ||
| AlertManagerEnabled AlertManagerMode = "Enabled" |
There was a problem hiding this comment.
Generally, it is recommended to have these constants follow a pattern of type name followed by the value. In this case it should be:
| AlertManagerEnabled AlertManagerMode = "Enabled" | |
| AlertManagerModeEnabled AlertManagerMode = "Enabled" |
| // /etc/alertmanager/secrets/<secret-name> within the 'alertmanager' container of | ||
| // the Alertmanager Pods. | ||
| // +optional | ||
| Secrets []string `json:"secrets,omitempty"` |
There was a problem hiding this comment.
For local object references, the api convention is to create a type alias. A good example is in the OpenShift API conventions doc under https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-specific-types-for-object-references-and-omit-ref-suffix
| // They will be added as volumes named secret-<secret-name> and mounted at | ||
| // /etc/alertmanager/secrets/<secret-name> within the 'alertmanager' container of | ||
| // the Alertmanager Pods. | ||
| // +optional |
There was a problem hiding this comment.
Mention that the field is optional in the godoc
| // tolerations Defines tolerations for the pods. | ||
| // +optional |
There was a problem hiding this comment.
Include the field is optional in the godoc. Elaborate further on the field, including what it does, why a user may care, etc.
| // topologySpreadConstraints Defines a pod's topology spread constraints. | ||
| // +optional |
There was a problem hiding this comment.
Same general comments regarding godoc updates.
| // volumeClaimTemplate Defines persistent storage for Alertmanager. Use this setting to | ||
| // configure the persistent volume claim, including storage class, volume | ||
| // size, and name. | ||
| // +optional |
There was a problem hiding this comment.
Mention the field is optional in the godoc. What happens when this field is unset?
Every component will be in a separated PR in order to improve review process
First PR: #1929
Related: Enhancements Proposal openshift/enhancements#1627