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

Add baremetal provisioning configuration to a new CR #119

Merged
merged 1 commit into from Dec 12, 2019
Merged

Add baremetal provisioning configuration to a new CR #119

merged 1 commit into from Dec 12, 2019

Conversation

sadasu
Copy link
Contributor

@sadasu sadasu commented Nov 15, 2019

Enhancement request details the configuration items that are going
to part of the new CR and the motivations for adding this CR.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 15, 2019
@sadasu
Copy link
Contributor Author

sadasu commented Nov 15, 2019

/cc @hardys
/cc @dhellmann

@sadasu
Copy link
Contributor Author

sadasu commented Nov 18, 2019

/cc @abhinavdahiya
/cc @enxebre
Would like you comments on this enhancement request.

@hardys
Copy link
Contributor

hardys commented Nov 27, 2019

This looks good to me now, only a couple of minor nits noted.

@abhinavdahiya can you please take a look when you get a moment so we can unblock the installer/API/MAO PRs related to this?

I think at this point this is as minimal as we can make this change just to configure the networking for the metal3 pod, we've removed the Image reference which you objected to in #90 so AFAICS this is now good to go.

@hardys hardys mentioned this pull request Dec 2, 2019
Closed
@aravindhp
Copy link
Contributor

/uncc @aravindhp

@openshift-ci-robot openshift-ci-robot removed the request for review from aravindhp December 4, 2019 16:05
This was referenced Dec 5, 2019
Closed
Closed
@sadasu sadasu force-pushed the metal3-provisioning-CR branch 2 times, most recently from 4e481cf to e1f9ed0 Compare December 10, 2019 18:10
@sadasu sadasu force-pushed the metal3-provisioning-CR branch 2 times, most recently from 289e9a0 to 970cb83 Compare December 10, 2019 19:54
@enxebre
Copy link
Member

enxebre commented Dec 11, 2019

@bison PTAL

@sadasu
Add baremetal provisioning configuration to a new CR
5dd2ca5 
Enhancement request details the configuration items that are going
to part of the new CR and the motivations for adding this CR.
enxebre
enxebre reviewed Dec 12, 2019
View reviewed changes
enhancements/baremetal/baremetal-provisioning-config.md
1. Which is a preferred name for the new CR "Metal3ProvisioningController" or
"Metal3Controller"?

[Closed] Based on review comments it appears that "Metal3ProvisioningController"
Copy link
Member

Choose a reason for hiding this comment

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

this is confusing, we are saying here https://github.com/openshift/enhancements/pull/119/files#diff-ab7b8857a0d8d7c60d9c74376123c4acR173 the name would be provisioningconfig.baremetal.operator.openshift.io

enxebre
enxebre reviewed Dec 12, 2019
View reviewed changes
enhancements/baremetal/baremetal-provisioning-config.md
which are not values that should be configured by the end user via BareMetalHost objects.

The configs described in this enhancement doc would be part of the Spec field of the CR.
Only the ProvisioningDHCP.DHCPRange field can change after installtion, so this will be
Copy link
Member

Choose a reason for hiding this comment

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

typo installtion

@enxebre
Copy link
Member

enxebre commented Dec 12, 2019 •
edited

/lgtm
other than https://github.com/openshift/enhancements/pull/119/files#r357027886
Thanks a lot @sadasu
@abhinavdahiya @bison ptal

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, sadasu

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 Dec 12, 2019
@openshift-merge-robot openshift-merge-robot merged commit 9847a15 into openshift:master Dec 12, 2019
@enxebre
Copy link
Member

enxebre commented Dec 12, 2019

not sure why the bot added the approval label, still #119 (comment)

@hardys hardys mentioned this pull request Dec 16, 2019
Closed
@stbenjam stbenjam mentioned this pull request Jan 23, 2020
Merged
@hardys hardys mentioned this pull request Apr 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhellmann dhellmann dhellmann left review comments

@stbenjam stbenjam stbenjam left review comments

@enxebre enxebre enxebre left review comments

@hardys hardys hardys left review comments

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@enxebre enxebre

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@sadasu @hardys @aravindhp @abhinavdahiya @enxebre @openshift-ci-robot @dhellmann @stbenjam @openshift-merge-robot