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.

Sign up for GitHub

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

Jump to bottom

Bug 1813369: Wrong key for selecting nodes in MachineConfigPool #4927

Merged
merged 1 commit into from Apr 8, 2020
Merged

Bug 1813369: Wrong key for selecting nodes in MachineConfigPool #4927

merged 1 commit into from Apr 8, 2020

Conversation

dtaylor113
Copy link
Contributor

@dtaylor113 dtaylor113 commented Apr 6, 2020 •
edited

Before

image
image

After

image
image

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 6, 2020
@openshift-ci-robot
Copy link
Contributor

@dtaylor113: This pull request references Bugzilla bug 1813369, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1813369: Wrong key for selecting nodes in MachineConfigPool

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Apr 6, 2020
@dtaylor113
Copy link
Contributor Author

/cherry-pick release-4.4

@openshift-cherrypick-robot
Copy link

@dtaylor113: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

spadgett
spadgett requested changes Apr 6, 2020
View reviewed changes
frontend/public/components/machine-config-pool.tsx Outdated
@@ -155,7 +155,7 @@ const MachineConfigPoolCounts: React.SFC<MachineConfigPoolCountsProps> = ({ obj

const MachineConfigPoolSummary: React.SFC<MachineConfigPoolSummaryProps> = ({ obj }) => {
const machineConfigSelector = _.get(obj, 'spec.machineConfigSelector');
const machineSelector = _.get(obj, 'spec.machineSelector');
const nodeSelector = _.get(obj, 'spec.nodeSelector');
return (
<ResourceSummary resource={obj}>
Copy link
Member

Choose a reason for hiding this comment

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

Pass showNodeSelector to ResourceSummary and remove the logic for it here.

@dtaylor113 dtaylor113 force-pushed the update-spec_nodeSelector-for-MachineConifigPool-template branch from baea3e6 to 54f6875 Compare April 6, 2020 17:08
spadgett
spadgett reviewed Apr 6, 2020
View reviewed changes
frontend/public/components/utils/details-page.tsx Outdated
Comment on lines 40 to 42
return obj.kind === 'Pod' || obj.kind === 'MachineConfigPool'
? 'spec.nodeSelector'
: 'spec.template.spec.nodeSelector';
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I forgot about this.

It would be better to pass the path in like we do for podSelector instead of hard-coding types here.

Copy link
Contributor Author

@dtaylor113 dtaylor113 Apr 6, 2020 •
edited

Choose a reason for hiding this comment

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

Hi @spadgett, fixed.
Updated last 'After' screenshot in PR description.

@openshift-ci-robot
Copy link
Contributor

@dtaylor113: This pull request references Bugzilla bug 1813369, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1813369: Wrong key for selecting nodes in MachineConfigPool

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dtaylor113 dtaylor113 force-pushed the update-spec_nodeSelector-for-MachineConifigPool-template branch 2 times, most recently from 55646b8 to e375bae Compare April 6, 2020 18:26
@openshift openshift deleted a comment from openshift-ci-robot Apr 6, 2020
@dtaylor113 dtaylor113 force-pushed the update-spec_nodeSelector-for-MachineConifigPool-template branch from e375bae to c83cbb7 Compare April 6, 2020 19:06
spadgett
spadgett reviewed Apr 6, 2020
View reviewed changes
frontend/public/components/utils/details-page.tsx Outdated
@@ -49,14 +49,14 @@ export const ResourceSummary: React.SFC<ResourceSummaryProps> = ({
showAnnotations = true,
showTolerations = false,
podSelector = 'spec.selector',
nodeSelector = '',
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
nodeSelector = '',
nodeSelector = 'spec.template.spec.nodeSelector',

frontend/public/components/utils/details-page.tsx Outdated
}) => {
const { metadata, type } = resource;
const reference = referenceFor(resource);
const model = modelFor(reference);
const nodeSelectorPath = getNodeSelectorPath(resource);
const nodeSelectorPath = _.isEmpty(nodeSelector) ? getNodeSelectorPath(resource) : nodeSelector;
Copy link
Member

Choose a reason for hiding this comment

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

Remove getNodeSelectorPath and pass it in for the one other exception we have for pods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett fixed as you suggested. Wish there was a way to search and be certain that there was only 'one other [Pod] exception'. I see some <ResourceSummary ../>'s use item.obj:
image
These are used by TopologyResourcePanel.tsx. Clicking around Topology viewer I can't bring up any Pod objects, but not 100% sure one can't.

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, just confirmed with @jeff-phillips-18 that Pods aren't items shown in the topology view. So passing item.obj to <ResourceSummary /> on the topology side panel is ok and we don't have to pass a nodeSelector path for Pods.
image

@dtaylor113 dtaylor113 force-pushed the update-spec_nodeSelector-for-MachineConifigPool-template branch from c83cbb7 to ef4877c Compare April 7, 2020 01:29
spadgett
spadgett approved these changes Apr 7, 2020
View reviewed changes
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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2020
@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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2020
@openshift-bot
Copy link
Contributor

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 6571daa into openshift:master Apr 8, 2020
@openshift-ci-robot
Copy link
Contributor

@dtaylor113: All pull requests linked via external trackers have merged: openshift/console#4927. Bugzilla bug 1813369 has been moved to the MODIFIED state.

In response to this:

Bug 1813369: Wrong key for selecting nodes in MachineConfigPool

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Apr 8, 2020
Merged
@openshift-cherrypick-robot
Copy link

@dtaylor113: new pull request created: #4963

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@spadgett spadgett added this to the v4.5 milestone Apr 14, 2020
@dtaylor113 dtaylor113 deleted the update-spec_nodeSelector-for-MachineConifigPool-template branch April 27, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@TheRealJon TheRealJon Awaiting requested review from TheRealJon

@rhamilto rhamilto Awaiting requested review from rhamilto

@zherman0 zherman0 Awaiting requested review from zherman0

Assignees

@spadgett spadgett

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dtaylor113 @openshift-ci-robot @openshift-cherrypick-robot @openshift-bot @spadgett @openshift-merge-robot