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

ironic provisioner de-duplicate image options #750

Merged
merged 1 commit into from
Jan 13, 2021
Merged

ironic provisioner de-duplicate image options #750

merged 1 commit into from
Jan 13, 2021

Conversation

hardys
Copy link
Member

@hardys hardys commented Dec 14, 2020

This aims to de-duplicate the code where ironic image settings are managed, so that new code planned for the LiveImage enhancement don't need to be added in two places

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 14, 2020
dhellmann
dhellmann reviewed Dec 14, 2020
View reviewed changes
Copy link
Member

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

This looks right to me. I don't know what sort of unit test coverage there is for getUpdateOptsForNode() that might need to be changed or expanded, but these changes look fine.

pkg/provisioner/ironic/ironic.go
p.host.HardwareProfile()))
}

func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image) (updates nodes.UpdateOpts, err error) {
// image_source
var op nodes.UpdateOp
if _, ok := ironicNode.InstanceInfo["image_source"]; !ok {
Copy link
Member

Choose a reason for hiding this comment

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

At some point we probably want this check to compare the value, not just see if there is one. That should be another PR, though, since it has bigger implications.

Copy link
Member

Choose a reason for hiding this comment

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

I have a patch for this but it needs tests: zaneb@ed324ac
Feel free to borrow.

@hardys
Copy link
Member Author

hardys commented Dec 14, 2020

This looks right to me. I don't know what sort of unit test coverage there is for getUpdateOptsForNode() that might need to be changed or expanded, but these changes look fine.

Thanks for the quick review - the test coverage looks OK to me, I could change that to refer to the new function explicitly - but I was thinking the fact that the existing tests all pass was a reasonable way to show that I hadn't broken things with the refactoring :)

@hardys
Copy link
Member Author

hardys commented Dec 15, 2020

/test-integration

@hardys
Copy link
Member Author

hardys commented Dec 15, 2020

/retitle ironic provisioner de-duplicate image options

@metal3-io-bot metal3-io-bot changed the title WIP ironic provisioner de-duplicate image options ironic provisioner de-duplicate image options Dec 15, 2020
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 15, 2020
@hardys hardys mentioned this pull request Dec 16, 2020
Closed
@hardys
Copy link
Member Author

hardys commented Jan 4, 2021

@dhellmann any more updates needed before we can merge this one?

/cc @zaneb

@dhellmann
Copy link
Member

I'm happy with these changes. I'll give @zaneb a chance to review them, too.

/approve

zaneb
zaneb reviewed Jan 11, 2021
View reviewed changes
pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved
pkg/provisioner/ironic/ironic.go Show resolved Hide resolved
@hardys hardys mentioned this pull request Jan 12, 2021
Merged
@hardys
Copy link
Member Author

hardys commented Jan 12, 2021

Ok I fixed the instance_uuid issue - from comments here and on #759 there are two potential further de-duplications:

Unless folks feel strongly otherwise I'll push follow-up PRs for those, since it's not strictly related to the image instance_info de-duplication I wanted to do before adding the live-iso code?

@hardys
Copy link
Member Author

hardys commented Jan 12, 2021

/test-integration

zaneb
zaneb reviewed Jan 12, 2021
View reviewed changes
Copy link
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

/approve

pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved
pkg/provisioner/ironic/ironic.go
p.host.HardwareProfile()))
}

func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image) (updates nodes.UpdateOpts, err error) {
// image_source
var op nodes.UpdateOp
if _, ok := ironicNode.InstanceInfo["image_source"]; !ok {
Copy link
Member

Choose a reason for hiding this comment

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

I have a patch for this but it needs tests: zaneb@ed324ac
Feel free to borrow.

pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, hardys, zaneb

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:
  • OWNERS [dhellmann,hardys,zaneb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot 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 Jan 13, 2021
@hardys
Copy link
Member Author

hardys commented Jan 13, 2021

/test-integration

@zaneb
Copy link
Member

zaneb commented Jan 13, 2021

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021
@metal3-io-bot metal3-io-bot merged commit ba4222d into metal3-io:master Jan 13, 2021
@hardys hardys mentioned this pull request Jan 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhellmann dhellmann Awaiting requested review from dhellmann

@zaneb zaneb Awaiting requested review from zaneb

Assignees
No one assigned
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.
4 participants
@hardys @dhellmann @metal3-io-bot @zaneb