Alertmanager: Added Email and Slack Receiver forms, implemented Save-As-Global/Default functionality #4041

dtaylor113 · 2020-01-22T16:54:49Z

Implemented 'Save as Default/Global' functionality, which defaults certain form fields to global values, and saves values in global section if 'Save as Default' checked. Includes new call to ${alertManagerBaseURL}/api/v2/status/ to get global values not found in alertmanager-main's alertmanager.yaml's global properties.
Added Email and Slack Receiver forms
Added new alertmanager integration test suite which tests initializing and saving global fields/values. Removed global testing from monitor integration test suite, does not need to run during CI.

Screenshots

Follow on [WIP] PR adds advanced fields: #4044

spadgett

I'm concerned that we have no type safety with this approach. This is problem particularly because we have no API server validation on this config (it's just a YAML block in a secret). I'm not sure what to suggest, though, because it we would need to change the approach to add types.

It seems like there's too much magic converting to/from Alertmanager config property names and camel case names. I'm not sure why that's necessary.

frontend/integration-tests/protractor.conf.ts

frontend/integration-tests/tests/alertmanager.scenario.ts

frontend/public/components/monitoring/receiver-forms/slack-receiver-form.tsx

dtaylor113 · 2020-01-23T18:15:10Z

Hi @spadgett, I believe I have addressed the majority of your review comments.
Remaining comments:

lack of typescripting for formValues and globals
~~need accessibility for blueInfoIcon~~
~~kubeadmin permissions for kubectl patch for afterAll() in alertmanager integration tests?~~

spadgett

Thanks, @dtaylor113. We need to make sure this can pass CI since I haven't seen it pass yet.

spadgett · 2020-01-24T17:49:24Z

frontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx

I don't think this is right... You want aria-describedby. This will break screenreaders using the label. I would leave this off.

spadgett · 2020-01-24T17:50:00Z

frontend/public/components/monitoring/_monitoring.scss

This shouldn't be necessary if you use the right checkbox styles.

This is not for the checkbox, it is for the blue (?) icon

This is not for the checkbox, it is for the blue (?) icon

We should give it a better name in that case.

I feel like we have some existing styles you can reuse for the tooltip icon.

Hi @spadgett, seaching through css, nothing jumping out at me, what's a better name? It matches the naming convention of the css definition above it in the .scss file.

spadgett · 2020-01-24T17:53:54Z

frontend/public/components/monitoring/receiver-forms/email-receiver-form.tsx

By convention, id should be kebab case, not camel case (here and below).

spadgett · 2020-01-24T21:10:56Z

/hold
for merge queue fix #4065

spadgett · 2020-01-24T23:31:13Z

/approve
/lgtm

We can address any further changes in follow on PRs.

spadgett · 2020-01-27T16:28:54Z

This was approved on Friday. Tagging again for the rebase

/lgtm

openshift-ci-robot · 2020-01-27T16:31:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtaylor113, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spadgett · 2020-01-27T20:47:21Z

/hold cancel

openshift-bot · 2020-01-27T21:12:37Z

/retest