Skip to content

Conversation

@dtaylor113
Copy link
Contributor

@dtaylor113 dtaylor113 commented Jan 22, 2020

Added advanced configuration fields within expand/collapse section on each receiver form.

Screenshots

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2020
@dtaylor113 dtaylor113 requested a review from spadgett January 22, 2020 19:54
@openshift-ci-robot 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 22, 2020
@dtaylor113 dtaylor113 changed the title [WIP] Alertmanager: Added advanced configuration fields [WIP] Alertmanager: Advanced configuration fields Jan 22, 2020
@dtaylor113 dtaylor113 force-pushed the alertmanager-advanced-configuration-fields branch 3 times, most recently from 94fda80 to 2f355a1 Compare January 24, 2020 20:19
@openshift openshift deleted a comment from openshift-ci-robot Jan 24, 2020
@dtaylor113 dtaylor113 force-pushed the alertmanager-advanced-configuration-fields branch from 2f355a1 to 251a43d Compare January 27, 2020 16:12
@spadgett spadgett added this to the v4.5 milestone Jan 29, 2020
@dtaylor113 dtaylor113 changed the base branch from master to release-4.5 January 29, 2020 13:59
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2020
@benjaminapetersen benjaminapetersen changed the title [WIP] Alertmanager: Advanced configuration fields [WIP] Alertmanager: Advanced configuration fields [4.5] Jan 29, 2020
@dtaylor113 dtaylor113 force-pushed the alertmanager-advanced-configuration-fields branch from 251a43d to 7b9b05e Compare January 29, 2020 17:06
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 29, 2020
@dtaylor113 dtaylor113 changed the title [WIP] Alertmanager: Advanced configuration fields [4.5] Alertmanager: Advanced configuration fields [4.5] Jan 29, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 29, 2020
@dtaylor113 dtaylor113 changed the base branch from release-4.5 to master March 10, 2020 19:52
@dtaylor113
Copy link
Contributor Author

Hi @benjaminapetersen, @kyoto, and @spadgett, I believe this PR is ready to be reviewed.

@benjaminapetersen
Copy link
Contributor

/hold

We need to know if the API is going to change on this, and if monitoring is working on that at this point. If it changes, we may want to revise to the new API rather than merging right now.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2020
@dtaylor113 dtaylor113 force-pushed the alertmanager-advanced-configuration-fields branch from 7b9b05e to 2ffc326 Compare March 25, 2020 14:17
@spadgett
Copy link
Member

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@spadgett spadgett changed the title Alertmanager: Advanced configuration fields [4.5] Alertmanager: Advanced configuration fields Apr 17, 2020
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Let's spell out the word. It's already a long name and editors have autocomplete.

Suggested change
export const showAdvConfiguration = $('button.pf-c-expandable__toggle');
export const showAdvancedConfiguration = $('button.pf-c-expandable__toggle');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either use the PF4 check or Bootstrap checkbox class instead of co-no-bold.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  shouldn't be needed if you use the checkbox component or Bootstrap checkbox classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title="Url"
title="URL"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the help blocks should have punctuation to be consistent with other forms in console (although it looks like existing help blocks for Alertmanager is missing that).

Suggested change
The url of the icon
The URL of the icon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Description of the incident
Description of the incident.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kebab case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Severity of the incident
Severity of the incident.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kebab case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use consistent case for all labels

Suggested change
Body of email notifications (HTML)
Body of Email Notifications (HTML)

@dtaylor113 dtaylor113 force-pushed the alertmanager-advanced-configuration-fields branch from 2ffc326 to 20ec311 Compare April 17, 2020 19:17
@dtaylor113
Copy link
Contributor Author

dtaylor113 commented Apr 17, 2020

Hi @spadgett, I believe I have addressed all of your review comments. Last commit has just those changes. Monitoring and Alertmanager integration tests passed locally -thanks

@dtaylor113 dtaylor113 force-pushed the alertmanager-advanced-configuration-fields branch from 20ec311 to 1bc5bb7 Compare April 17, 2020 20:21
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot
Copy link
Contributor

[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:

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

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 17, 2020
@spadgett
Copy link
Member

/hold
for #5100

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@dtaylor113
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit d2c48b4 into openshift:master Apr 18, 2020
@dtaylor113 dtaylor113 deleted the alertmanager-advanced-configuration-fields branch April 27, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/monitoring Related to monitoring lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants