Skip to content

Add/Edit BMH - make BMC optional#5942

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
rawagner:bmh_credentials_e
Jul 13, 2020
Merged

Add/Edit BMH - make BMC optional#5942
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
rawagner:bmh_credentials_e

Conversation

@rawagner
Copy link
Copy Markdown
Contributor

@rawagner rawagner commented Jul 10, 2020

Screenshot from 2020-07-10 12-29-14
Screenshot from 2020-07-10 12-29-05

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Jul 10, 2020
@openshift-ci-robot openshift-ci-robot added component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/olm Related to OLM approved Indicates a PR has been approved by an approver from all required OWNERS files. component/shared Related to console-shared labels Jul 10, 2020
rawagner
rawagner commented Jul 10, 2020
View reviewed changes
Comment thread frontend/packages/metal3-plugin/src/hooks/index.ts Outdated
Copy link
Copy Markdown
Contributor Author

@rawagner rawagner Jul 10, 2020

Choose a reason for hiding this comment

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

@jtomasek I moved this hook to shared and also simplified it a bit.

Copy link
Copy Markdown
Contributor Author

@rawagner rawagner Jul 10, 2020

Choose a reason for hiding this comment

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

FIY useEffect is async and storing value to ref wont trigger a component update. Which means for first render this hook returns undefined and second render (which have to be triggered by something else) will return the correct value.

Copy link
Copy Markdown
Contributor Author

@rawagner rawagner Jul 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think its usually fine. But there's no way to update the previous value (after all its called previous, not first). Once you try to add dependency array to be able to update the initial value the async nature of useEffect will cause issues.

@rawagner
Copy link
Copy Markdown
Contributor Author

rawagner commented Jul 10, 2020

@andybraren @jtomasek

@rawagner rawagner force-pushed the bmh_credentials_e branch from 540f68b to 53b8572 Compare July 10, 2020 10:45
andybraren
andybraren reviewed Jul 10, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@andybraren andybraren left a comment

Choose a reason for hiding this comment

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

Looks great!

Just to confirm the behavior:

  • If the user clicks Edit Host in the host's action menu, is the Enable power management checkbox checked/unchecked based on whether the BareMetalHost contains BMC credentials (checked if there are credentials, unchecked if there aren't)?
  • If the user clicks Add credentials anywhere in #5830, is the Enable power management checkbox automatically checked so that the Username/Password fields are visible?

And while we're in here, maybe two minor wording tweaks (missing a's):

Expand the hardware inventory by registering a new Bare Metal Host.

Provide a unique name...

@rawagner
Copy link
Copy Markdown
Contributor Author

rawagner commented Jul 10, 2020 •
edited
Loading

Looks great!

Just to confirm the behavior:

  • If the user clicks Edit Host in the host's action menu, is the Enable power management checkbox checked/unchecked based on whether the BareMetalHost contains BMC credentials (checked if there are credentials, unchecked if there aren't)?
  • If the user clicks Add credentials anywhere in Add no power management status and alert for BMH #5830, is the Enable power management checkbox automatically checked so that the Username/Password fields are visible?

correct :)

And while we're in here, maybe two minor wording tweaks (missing a's):

Expand the hardware inventory by registering a new Bare Metal Host.

Provide a unique name...

fixed

@rawagner rawagner force-pushed the bmh_credentials_e branch from 53b8572 to e88c823 Compare July 10, 2020 14:27
@rawagner
Update BMH add/edit dialog
9394753 
 - option to disable/enable power mgmt
 - handle loading/error states
@rawagner rawagner force-pushed the bmh_credentials_e branch from e88c823 to 9394753 Compare July 10, 2020 14:31
andybraren
andybraren approved these changes Jul 10, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@andybraren andybraren left a comment

Choose a reason for hiding this comment

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

correct :)

I'm happy then 😄 LGTM!

fixed

🎉

@jtomasek
Copy link
Copy Markdown

jtomasek commented Jul 13, 2020

/lgtm

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

openshift-ci-robot commented Jul 13, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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

openshift-bot commented Jul 13, 2020

/retest

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

4 similar comments
@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Jul 13, 2020

/retest

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

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Jul 13, 2020

/retest

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

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Jul 13, 2020

/retest

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

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Jul 13, 2020

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 852e22f into openshift:master Jul 13, 2020
@spadgett spadgett added this to the v4.6 milestone Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcaianirh jcaianirh Awaiting requested review from jcaianirh

@rhrazdil rhrazdil Awaiting requested review from rhrazdil

2 more reviewers

@jtomasek jtomasek jtomasek approved these changes

@andybraren andybraren andybraren approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jtomasek jtomasek

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/olm Related to OLM component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

v4.6

Development

Successfully merging this pull request may close these issues.

7 participants

@rawagner @jtomasek @openshift-ci-robot @openshift-bot @andybraren @spadgett @openshift-merge-robot