Skip to content

Conversation

@rawagner
Copy link
Contributor

@rawagner rawagner commented Jul 17, 2019

@jtomasek @honza I haven't tried it so if you can, it would be great

verified, thanks @honza
bmh_inventory

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 17, 2019
@rawagner rawagner force-pushed the host-overview branch 2 times, most recently from bbbf254 to f5c51fd Compare July 18, 2019 11:25
@rawagner
Copy link
Contributor Author

@honza @jtomasek take a look

Choose a reason for hiding this comment

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

HOST_REGISTERING_STATES and HOST_PROVISIONING_STATES are not needed as those are a subset of HOST_PROGRESS_STATES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know, removed redundant states

Choose a reason for hiding this comment

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

I believe the statusIDs be the host status constants from ../../constants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these statusIDs are keys in this object https://github.com/openshift/console/blob/master/frontend/packages/metal3-plugin/src/components/table-filters.ts#L14 . Most of them are in constants (missing other) but It seems to me that those constants are values which can appear in bmh status K8s object and are not meant to be IDs of status filters. Correct me if Im wrong.

Choose a reason for hiding this comment

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

You're right, thanks for clarification.

Choose a reason for hiding this comment

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

This function does not exist any more. getBMHStatusGroups will now need to take bmh and nodeMaintenance to correctly identify host status. See https://github.com/openshift/console/blob/master/frontend/packages/metal3-plugin/src/utils/host-status.ts#L57

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function updated. Im fetching bmhs, nodes, hosts and node maintenances now

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 18, 2019
Copy link

@jtomasek jtomasek left a comment

Choose a reason for hiding this comment

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

Looks good, 2 nits

Choose a reason for hiding this comment

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

Nit: renaming bmh to host would align to what we're using in other places for baremetal host variable and simplify

Suggested change
const hostMultiStatus = getHostStatus({ host: bmh, nodeMaintenance });
const hostMultiStatus = getHostStatus({ host, nodeMaintenance });

Choose a reason for hiding this comment

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

same nit as above

Choose a reason for hiding this comment

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

You're right, thanks for clarification.

@rawagner
Copy link
Contributor Author

renamed bmh to host to align with rest of metal3 code.

@jtomasek
Copy link

/lgtm

@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 Jul 18, 2019
@openshift-bot
Copy link
Contributor

/retest

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

@spadgett spadgett added this to the v4.2 milestone Jul 18, 2019
@spadgett
Copy link
Member

/retest

3 similar comments
@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

/retest

@rawagner
Copy link
Contributor Author

/test e2e-aws
/test e2e-aws-console-olm

1 similar comment
@rawagner
Copy link
Contributor Author

/test e2e-aws
/test e2e-aws-console-olm

@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2019
@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@spadgett
Copy link
Member

/retest

@rawagner
Copy link
Contributor Author

/test e2e-aws-console-olm
/test e2e-aws-console

@openshift-bot
Copy link
Contributor

/retest

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

9 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-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-bot
Copy link
Contributor

/retest

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

@spadgett
Copy link
Member

/hold

It looks something in this pr has broken the tests

@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 Jul 22, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 22, 2019
@rawagner
Copy link
Contributor Author

rawagner commented Jul 22, 2019

Dashboards page failed to load because there is some circular dependency when loading code from plugin.ts which depends on some code from components/utils. In this case the issue appeared after merging #1697. plugin.ts depends on code which is loading Status from console/shared and that depends on code from components/utils so I had to change imports of console/shared to more specific ones ie console/shared/src/selectors.

This is not specific to Status component as I noticed similar issue when plugin.ts depends on code from components/utils a few times before. We will need to take a closer look at the issue in the future.

@rawagner
Copy link
Contributor Author

/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 Jul 22, 2019
@atiratree
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtomasek, rawagner, suomiy, vojtechszocs

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-merge-robot openshift-merge-robot merged commit 0084ea6 into openshift:master Jul 22, 2019
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. 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.

8 participants