Skip to content
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

Moved Alertmanager Config & YAML to Admin -> Cluster Settings -> Global Configuration #3159

Conversation

dtaylor113
Copy link
Contributor

image

image

image

@dtaylor113 dtaylor113 requested a review from kyoto October 30, 2019 18:12
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 30, 2019
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Oct 30, 2019
@@ -143,7 +167,7 @@ class GlobalConfigPage_ extends React.Component<GlobalConfigPageProps, GlobalCon
</div>
<div className="co-m-table-grid__body">
{_.map(sortedItems, (item) => (
<ItemRow item={item} key={item.uid} />
<ItemRow item={item} key={_.get(item, 'uid')} />
Copy link
Member

Choose a reason for hiding this comment

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

If we need to use _.get, it means not everything has a UID, which means not all rows have keys. That's bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

const description = getResourceDescription(item.model);

let resourceLink, menuItems, description;
if (item.isK8Item !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the right place to have this logic. It would be better to pass in an item object with link, actions, and description already set. Then the logic here is trivial. The way to build those items should be moved up to a higher level that already knows what kind of config link it's working with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

frontend/public/components/nav/admin-nav.tsx Show resolved Hide resolved
@dtaylor113 dtaylor113 force-pushed the alerting-config-to-cluster-settings branch from b4c33cb to 8f076c2 Compare October 30, 2019 19:27
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.

Let's change the names of some of the item properties to be something more generic and only transform the config items once instead of twice.

usableConfigs.length > 0 ? _.sortBy(_.flatten(allItems), 'kind', 'asc') : items;

let allItems = items.concat(usableConfigs);
allItems = allItems
Copy link
Member

Choose a reason for hiding this comment

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

const allItems = [...items, ...usableConfgs].map((item) => {

const apiExplorerLink = `/api-resource/cluster/${referenceForModel(item.model)}`;
const resourceLink = resourcePathFromModel(item.model, item.name, item.namespace);
return {
...item,
Copy link
Member

Choose a reason for hiding this comment

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

Don't pass more than is necessary. You should be able to remove this line. I would add a label prop to the row items since kind is not accurate when it's not a k8s resource.

label: item.kind,
id: item.uid,
path: resourceLink,

};
})
.concat({
kind: 'Alertmanager',
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
kind: 'Alertmanager',
label: 'Alertmanager',

})
.concat({
kind: 'Alertmanager',
uid: 'alertmanager',
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a UID. Let's just call it id

Suggested change
uid: 'alertmanager',
id: 'alertmanager',

kind: 'Alertmanager',
uid: 'alertmanager',
description: 'Configure grouping and routing of alerts',
resourceLink: '/monitoring/alertmanagerconfig',
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this path instead of resourceLink. It's not always a resource.

Suggested change
resourceLink: '/monitoring/alertmanagerconfig',
path: '/monitoring/alertmanagerconfig',


let allItems = items.concat(usableConfigs);
allItems = allItems
.map((item) => {
Copy link
Member

Choose a reason for hiding this comment

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

We're already mapping in componentDidMount. We shouldn't transform it twice. move this logic there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, moved all to componentDidMount, thanks

@dtaylor113 dtaylor113 force-pushed the alerting-config-to-cluster-settings branch from 8f076c2 to 916b11a Compare October 31, 2019 13:39
@openshift openshift deleted a comment from openshift-ci-robot Oct 31, 2019
@dtaylor113
Copy link
Contributor Author

/retest

@openshift openshift deleted a comment from openshift-ci-robot Oct 31, 2019
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.

/approve
/lgtm

Thanks @dtaylor113, this looks cleaner


const globalConfigs = this.getGlobalConfigs();
const usableConfigs = globalConfigs
.filter((item) => this.checkFlags(item))
Copy link
Member

Choose a reason for hiding this comment

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

There's an edge case where the flags can change after the component mounts. We might have to keep the flags in the item and filter in render. We can address in a follow-on.

@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 Oct 31, 2019
@openshift openshift deleted a comment from openshift-ci-robot Nov 1, 2019
@spadgett spadgett added this to the v4.3 milestone Nov 1, 2019
@spadgett
Copy link
Member

spadgett commented Nov 2, 2019

@openshift-ci-robot openshift-ci-robot removed 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 Nov 2, 2019
@spadgett
Copy link
Member

spadgett commented Nov 4, 2019

@dtaylor113 i believe the tests need to be updated for these changes

@dtaylor113 dtaylor113 force-pushed the alerting-config-to-cluster-settings branch from 916b11a to 9e7ecb3 Compare November 4, 2019 17:36
@openshift openshift deleted a comment from openshift-ci-robot Nov 4, 2019
@openshift openshift deleted a comment from openshift-ci-robot Nov 4, 2019
@openshift openshift deleted a comment from openshift-ci-robot Nov 5, 2019
@openshift openshift deleted a comment from spadgett Nov 5, 2019
@openshift openshift deleted a comment from spadgett Nov 5, 2019
@openshift openshift deleted a comment from spadgett Nov 5, 2019
@dtaylor113
Copy link
Contributor Author

@spadgett, think this is ready to merge -thanks

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.

/approve
/lgtm

@spadgett spadgett added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Nov 5, 2019
@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

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 openshift deleted a comment from openshift-ci-robot Nov 5, 2019
@openshift openshift deleted a comment from openshift-bot Nov 5, 2019
@dtaylor113
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 77fee11 into openshift:master Nov 5, 2019
@dtaylor113 dtaylor113 deleted the alerting-config-to-cluster-settings branch April 27, 2020 14:16
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 lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants