Alertmanager: Refactored #3937

dtaylor113 · 2020-01-13T17:39:23Z

Refactored:

moved single-use functions out of utils, per @kyoto 's comment
updated alert-manager-yaml-editor to use new getAlertManagerYAML method in utils
moved sub-forms to separate files
updated ReceiverBaseForm, , per @kyoto 's comment, to:
- clearly show INIT value handling
- generically handle subforms via subFormFactory
- implemended useFormStore custom hook for generic storing of form values

spadgett

Hey @dtaylor113, I'm still working through this, but wanted to get you some initial comments. I'm a little concerned with this approach since we lose type safety.

frontend/integration-tests/protractor.conf.ts

frontend/public/components/app-contents.tsx

frontend/public/components/monitoring/alert-manager-utils.tsx

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

frontend/public/components/monitoring/alert-manager-utils.tsx

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

dtaylor113 · 2020-01-14T15:43:42Z

Hi @spadgett , I believe I have addressed the majority of your initial review comments. Please see last commit for these updates, and additional comments above for unresolved issues. For fixed comments, I tag each comment as 'fixed' then resolve the conversation -let me know if this is ok for you, I feel it cleans up the code review and just leaves any open issues visible, but it could be annoying to you so I'm just checking :-)

dtaylor113 · 2020-01-16T14:34:41Z

/retest

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

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

frontend/public/components/utils/dropdown.jsx

frontend/integration-tests/views/cluster-settings.view.ts

frontend/public/components/monitoring/receiver-forms/routing-labels-editor.tsx

dtaylor113 · 2020-01-17T20:24:01Z

Hi @spadgett, thanks for round of comments, I believe I have addressed them. Please see last commits for changes.

frontend/public/components/monitoring/receiver-forms/routing-labels-editor.tsx

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

frontend/integration-tests/protractor.conf.ts

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

spadgett · 2020-01-21T14:28:50Z

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

+            dropDownClassName="dropdown--full-width"
+            data-test-id="receiver-type"
+            selectedKey={formValues.receiverType}
+            onChange={(e) =>


This isn't an event, so e as a name is misleading.

Suggested change

onChange={(e) =>

onChange={(receiverType) =>

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

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

dtaylor113 · 2020-01-21T18:23:51Z

/retest

spadgett

Thanks @dtaylor113. This is looking a lot cleaner 👍

spadgett · 2020-01-21T19:38:01Z

frontend/integration-tests/views/form.view.ts

@@ -0,0 +1,3 @@
+import { $ } from 'protractor';
+
+export const firstDropdownMenuByTestID = (id: string) => $(`[data-test-dropdown-menu=${id}]`);


Drop the first prefix :)

Suggested change

export const firstDropdownMenuByTestID = (id: string) => $(`[data-test-dropdown-menu=${id}]`);

export const dropdownMenuForID = (id: string) => $(`[data-test-dropdown-menu=${id}]`);

spadgett · 2020-01-21T19:40:25Z

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

+  }
+};
+
+function formReducer(formValues, action) {


Suggested change

function formReducer(formValues, action) {

const formReducer = (formValues, action) => {

Would be nice to have types here

spadgett · 2020-01-21T19:41:22Z

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

+    config = getAlertmanagerConfig(secret, setErrorMsg);
+  }
+
+  const INITIAL_STATE = {


nit: why all caps in the name?

Just so it stands out more as it's being setup in the next couple of code blocks

spadgett · 2020-01-21T19:42:40Z

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

+
+  const isFormInvalid =
+    _.isEmpty(formValues.receiverName) ||
+    _.isEmpty(formValues.receiverType) ||


If these are strings, you don't need to use _.isEmpty, just !formValues.receiverName

yeah, I had these fixed in large PR we closed, it was one of Ben's initial comments. Lost them in recreating this PR. fixed

spadgett · 2020-01-21T19:44:02Z

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

+};
+
+export const isFormInvalid = (formValues): boolean => {
+  return _.isEmpty(formValues.pagerDutyIntegrationKey);


This is just a string, right?

Suggested change

return _.isEmpty(formValues.pagerDutyIntegrationKey);

return !formValues.pagerDutyIntegrationKey;

spadgett · 2020-01-21T19:45:16Z

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

+  return _.isEmpty(receiverConfig)
+    ? { webhookUrl: '' }
+    : {
+        webhookUrl: receiverConfig?.url,
+      };


this could be

return { webhookURL: receiverConfig?.url || '' };

If this is not a prop in the Alertmanager config, use webhookURL

spadgett · 2020-01-21T19:45:28Z

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

+};
+
+export const isFormInvalid = (formValues) => {
+  return _.isEmpty(formValues.webhookUrl);


Suggested change

return _.isEmpty(formValues.webhookUrl);

return !formValues.webhookUrl;

spadgett

/approve
/lgtm

Thanks for your patience @dtaylor113

spadgett · 2020-01-21T21:07:42Z

/hold

@dtaylor113 do you mind squashing?

…of utils

spadgett · 2020-01-21T21:11:40Z

/approve
/lgtm
/hold cancel

openshift-ci-robot · 2020-01-21T21:13:26Z

[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

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

openshift-bot · 2020-01-21T23:08:18Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2020-01-21T23:17:19Z

/hold
the merge queue is blocked

spadgett · 2020-01-22T01:31:19Z

/hold cancel
/retest

dtaylor113 requested review from benjaminapetersen and spadgett January 13, 2020 17:39

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/core Related to console core functionality component/monitoring Related to monitoring labels Jan 13, 2020

openshift-ci-robot requested review from kyoto and TheRealJon January 13, 2020 17:40

spadgett reviewed Jan 13, 2020

View reviewed changes

dtaylor113 force-pushed the alertmanager-just-refactoring branch from 103f9fa to efd460d Compare January 13, 2020 19:03

openshift deleted a comment from openshift-ci-robot Jan 14, 2020

dtaylor113 force-pushed the alertmanager-just-refactoring branch from 343dad8 to 671c816 Compare January 14, 2020 18:19

openshift deleted a comment from openshift-ci-robot Jan 16, 2020

dtaylor113 force-pushed the alertmanager-just-refactoring branch from 671c816 to 13e974d Compare January 16, 2020 20:33

spadgett reviewed Jan 17, 2020

View reviewed changes

dtaylor113 force-pushed the alertmanager-just-refactoring branch from 13e974d to 0fd6955 Compare January 17, 2020 20:15

dtaylor113 force-pushed the alertmanager-just-refactoring branch from 832758b to 5a2398f Compare January 17, 2020 21:38

spadgett requested changes Jan 17, 2020

View reviewed changes

dtaylor113 force-pushed the alertmanager-just-refactoring branch from 5a2398f to 1902b50 Compare January 20, 2020 16:55

spadgett reviewed Jan 21, 2020

View reviewed changes

dtaylor113 force-pushed the alertmanager-just-refactoring branch 2 times, most recently from 1a39279 to c68a971 Compare January 21, 2020 16:07

openshift deleted a comment from openshift-ci-robot Jan 21, 2020

dtaylor113 force-pushed the alertmanager-just-refactoring branch 2 times, most recently from cc32a5a to 1a66fee Compare January 21, 2020 16:31

openshift deleted a comment from openshift-ci-robot Jan 21, 2020

spadgett reviewed Jan 21, 2020

View reviewed changes

dtaylor113 force-pushed the alertmanager-just-refactoring branch from 1a66fee to d162c1e Compare January 21, 2020 20:40

spadgett approved these changes Jan 21, 2020

View reviewed changes

openshift-ci-robot assigned spadgett Jan 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

spadgett added this to the v4.4 milestone Jan 21, 2020

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 21, 2020

Refactoring: useReceiverFormStore and move single used functions out …

da62eb0

…of utils

dtaylor113 force-pushed the alertmanager-just-refactoring branch from d162c1e to da62eb0 Compare January 21, 2020 21:11

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 21, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2020

openshift-merge-robot merged commit 013a01c into openshift:master Jan 22, 2020

dtaylor113 deleted the alertmanager-just-refactoring branch April 27, 2020 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alertmanager: Refactored #3937

Alertmanager: Refactored #3937

dtaylor113 commented Jan 13, 2020

spadgett left a comment

dtaylor113 commented Jan 14, 2020 •

edited

dtaylor113 commented Jan 16, 2020

dtaylor113 commented Jan 17, 2020 •

edited

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020

dtaylor113 commented Jan 21, 2020

spadgett left a comment

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020 •

edited

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020

spadgett Jan 21, 2020

dtaylor113 Jan 21, 2020

spadgett left a comment

spadgett commented Jan 21, 2020

spadgett commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

openshift-bot commented Jan 21, 2020

spadgett commented Jan 21, 2020

spadgett commented Jan 22, 2020

		@@ -0,0 +1,3 @@
		import { $ } from 'protractor';

		export const firstDropdownMenuByTestID = (id: string) => $(`[data-test-dropdown-menu=${id}]`);

	export const firstDropdownMenuByTestID = (id: string) => $(`[data-test-dropdown-menu=${id}]`);
	export const dropdownMenuForID = (id: string) => $(`[data-test-dropdown-menu=${id}]`);

	function formReducer(formValues, action) {
	const formReducer = (formValues, action) => {

	return _.isEmpty(formValues.pagerDutyIntegrationKey);
	return !formValues.pagerDutyIntegrationKey;

	return _.isEmpty(formValues.webhookUrl);
	return !formValues.webhookUrl;

Alertmanager: Refactored #3937

Alertmanager: Refactored #3937

Conversation

dtaylor113 commented Jan 13, 2020

Refactored:

spadgett left a comment

Choose a reason for hiding this comment

dtaylor113 commented Jan 14, 2020 • edited

dtaylor113 commented Jan 16, 2020

dtaylor113 commented Jan 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtaylor113 commented Jan 21, 2020

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtaylor113 Jan 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Jan 21, 2020

spadgett commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

openshift-bot commented Jan 21, 2020

spadgett commented Jan 21, 2020

spadgett commented Jan 22, 2020

dtaylor113 commented Jan 14, 2020 •

edited

dtaylor113 commented Jan 17, 2020 •

edited

dtaylor113 Jan 21, 2020 •

edited