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

Allow setting properties and root_device #10

Merged
merged 2 commits into from
Mar 29, 2019

Conversation

hardys
Copy link

@hardys hardys commented Mar 27, 2019

Fixes #8

Copy link
Member

@stbenjam stbenjam 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 - a couple of comments below

ironic/resource_ironic_node_v1.go Outdated Show resolved Hide resolved
ironic/resource_ironic_node_v1.go Show resolved Hide resolved
ironic/resource_ironic_node_v1.go Show resolved Hide resolved
Due to limitations in terraform, we can't pass a nested map here
so instead surface the root_device as a top-level interface, then
set the map value internally.

More info at hashicorp/terraform#2114

Fixes: openshift-metal3#8
hardys pushed a commit to hardys/dev-scripts that referenced this pull request Mar 28, 2019
This is required for enabling extra disks, e.g for rook/ceph

This also enables an extra disk so we can prove this works
and support testing with ceph/rook.

Note this requires openshift-metal3/terraform-provider-ironic#10
@hardys hardys requested a review from stbenjam March 28, 2019 11:48
hardys pushed a commit to hardys/dev-scripts that referenced this pull request Mar 28, 2019
This is required for enabling extra disks, e.g for rook/ceph

This also enables an extra disk so we can prove this works
and support testing with ceph/rook.

Note this requires openshift-metal3/terraform-provider-ironic#10
Copy link
Member

@stbenjam stbenjam left a comment

Choose a reason for hiding this comment

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

LGTM!

We'll need to add root_device to the installer instead of utils.sh as well

@hardys
Copy link
Author

hardys commented Mar 28, 2019

We'll need to add root_device to the installer instead of utils.sh as well

Good point, I added it to utils.sh assuming that patch may land before the switch to kni-installer.

Are you planning a rebase to the kni-installer patch, or should I post a follow-up?

I guess we'll need to expose this via install-config for the baremetal platform as well.

@stbenjam
Copy link
Member

If this goes first I'll update the kni-installer PR's to include root_device

hardys pushed a commit to hardys/dev-scripts that referenced this pull request Mar 28, 2019
This is required for enabling extra disks, e.g for rook/ceph

This also enables an extra disk so we can prove this works
and support testing with ceph/rook.

Note this requires openshift-metal3/terraform-provider-ironic#10
hardys pushed a commit to hardys/dev-scripts that referenced this pull request Mar 29, 2019
This is required for enabling extra disks, e.g for rook/ceph

This also enables an extra disk so we can prove this works
and support testing with ceph/rook.

Note this requires openshift-metal3/terraform-provider-ironic#10
hardys pushed a commit to hardys/dev-scripts that referenced this pull request Mar 29, 2019
This is required for enabling extra disks, e.g for rook/ceph

This also enables an extra disk so we can prove this works
and support testing with ceph/rook.

Note this requires openshift-metal3/terraform-provider-ironic#10
hardys pushed a commit to hardys/dev-scripts that referenced this pull request Mar 29, 2019
This is required for enabling extra disks, e.g for rook/ceph

This also enables an extra disk so we can prove this works
and support testing with ceph/rook.

Note this requires openshift-metal3/terraform-provider-ironic#10
@hardys hardys merged commit 7cf70fc into openshift-metal3:master Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants