Alerting: Configuration tab, page, and Edit modal - #2783
Conversation
openshift-ci-robot
added
component/core
|
@kyoto are you able to help review? @dtaylor113 thanks. i haven't reviewed closely, but one comment: we should prefer functional components with hooks for new components instead of class components |
6c4e726 to
69b54e1
Compare
Changed to use functional component with hooks. |
|
/retest |
There was a problem hiding this comment.
Could these _.cloneDeep calls be moved inside submit instead of using a hook?
There was a problem hiding this comment.
Nit: className attribute should be on its own line.
(and same for the inputs below)
There was a problem hiding this comment.
Nit: Inconsistent spacing for import { ... }. I think we generally put space chars inside the {}. (Maybe we can add an eslint rule.)
There was a problem hiding this comment.
Maybe just inline this in AlertRouting since it is so simple?
69b54e1 to
febe9c0
Compare
|
Hi @kyoto, thanks for the review. I believe I have addressed all the issues. |
There was a problem hiding this comment.
I think this className got moved to its own line by mistake?
There was a problem hiding this comment.
Probably shouldn't be using an inline style.
There was a problem hiding this comment.
Actually, it looks to me like we don't need to clone secret anyway?
There was a problem hiding this comment.
Should probably assign this to a new variable, rather than overwriting config.
There was a problem hiding this comment.
This className attribute should be moved to its own line too.
febe9c0 to
1f78a5a
Compare
|
Hi @kyoto, addressed latest review comments -thanks |
| const alertPath = '/monitoring/alerts'; | ||
| const silencePath = '/monitoring/silences'; | ||
| const YAMLPath = '/monitoring/alertmanageryaml'; | ||
| const ConfigPath = '/monitoring/alertmanagerconfig'; |
There was a problem hiding this comment.
Nit: Should be configPath (camel case)
|
I notice there's no field validation for the modal. Is that something we're planning to add? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtaylor113, kyoto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
5 participants
New Configuration tab & Alert Routing section
New Edit Alert Routing modal
Design Doc: https://github.com/openshift/openshift-origin-design/blob/b85c195d30f9a588570672d8e5f9a7ce9edbac28/web-console/monitoring/alertmanager-config/alertmanager-config.md