Skip to content

Support for sending full ignition to masters#38

Merged
stbenjam merged 1 commit into
openshift-metal3:masterfrom
kirankt:full-ignition
Mar 31, 2020
Merged

Support for sending full ignition to masters#38
stbenjam merged 1 commit into
openshift-metal3:masterfrom
kirankt:full-ignition

Conversation

@kirankt

@kirankt kirankt commented Mar 5, 2020

Copy link
Copy Markdown
Contributor

Continuing the work of @hardys in #37 .

This PR adds support for https cert disabling and retries.

Comment thread ironic/resource_ironic_deployment.go Outdated
@kirankt

kirankt commented Mar 6, 2020

Copy link
Copy Markdown
Contributor Author

Looks like CI is failing. Do we need to bump CI's go version to 1.13?

@stbenjam stbenjam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread ironic/resource_ironic_deployment.go
@kirankt

kirankt commented Mar 10, 2020

Copy link
Copy Markdown
Contributor Author

Looks good to me, a couple questions.

Could we add a test?

https://github.com/openshift-metal3/terraform-provider-ironic/blob/master/ironic/resource_ironic_deployment_test.go

Absolutely. Will work on it.

@kirankt

kirankt commented Mar 30, 2020

Copy link
Copy Markdown
Contributor Author

@stbenjam Please take a look at the new changes. I have removed user-data field because we're not going to pass it from the installer side anymore.

@stbenjam

Copy link
Copy Markdown
Member

@stbenjam Please take a look at the new changes. I have removed user-data field because we're not going to pass it from the installer side anymore.

It doesn't look like it's fully removed, but I think it should probably stay as an option anyway. OpenShift may not be the only user of this, and I'd like to not break them. It can be exclusive, make sure user_data or the URL are set, but not both.

Update go.mod to fix go-systemd error

Add full ignition support with HTTP retries with TLS support

Add unit test for fetching full ignition
@kirankt

kirankt commented Mar 30, 2020

Copy link
Copy Markdown
Contributor Author

@stbenjam Please take a look at the new changes. I have removed user-data field because we're not going to pass it from the installer side anymore.

It doesn't look like it's fully removed, but I think it should probably stay as an option anyway. OpenShift may not be the only user of this, and I'd like to not break them. It can be exclusive, make sure user_data or the URL are set, but not both.

I just retained what we did earlier. Give user_data_url higher priority when both are specified. if there is an error getting data from user_data_url, we fall back to user_data.

@stbenjam stbenjam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks

@stbenjam stbenjam merged commit 667e1c6 into openshift-metal3:master Mar 31, 2020
@kirankt kirankt deleted the full-ignition branch April 1, 2020 21:53
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.

3 participants