-
Notifications
You must be signed in to change notification settings - Fork 57
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
fix: add description field to CSV base #366
fix: add description field to CSV base #366
Conversation
Signed-off-by: Mario Fernandez <[email protected]>
@@ -59,40 +59,68 @@ spec: | |||
apiservicedefinitions: {} | |||
customresourcedefinitions: | |||
owned: | |||
- kind: AlertmanagerConfig | |||
- description: AlertmanagerConfig defines a namespaced AlertmanagerConfig to be |
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.
Did you take a look at the UI rendering? I think it should be shorter to fit into the box, like in Gabriel's example
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.
It is ok, thanks!
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.
You can use https://operatorhub.io/preview to preview the bundle/csv.yaml
I think we can improve the AlertmanagerConfig
a bit .. (ask chatgpt :D )
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.
BTW, are we now supporting all these API?
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.
"AlertmanagerConfig is used to define configurations for Prometheus Alertmanager, specifying how alerts are handled and routed for notifications."
Something like this?
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.
@simonpasquier would be the best judge of this :) .. BTW, for the record, this was only a suggestion and shouldn't block the PR from merging.
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.
small suggestion compared to @marioferh
... specifying how alerts are grouped and notified to external systems.
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.
The descriptions are not truncated and the boxes don't look text-heavy. /lgtm |
1506632
to
df31425
Compare
Signed-off-by: Mario Fernandez <[email protected]>
df31425
to
447fa95
Compare
/lgtm |
Signed-off-by: Mario Fernandez <[email protected]>
Thanks! I guess there is nothing but keeping these up-to-date manually is there? |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f, marioferh 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 |
Add description field to CSV base of every owned CRD to generate correct description when run bundle generate.