Skip to content

remove convenience methods from BareMetalHost#741

Merged
metal3-io-bot merged 3 commits intometal3-io:masterfrom
dhellmann:drop-available-method
Jan 11, 2021
Merged

remove convenience methods from BareMetalHost#741
metal3-io-bot merged 3 commits intometal3-io:masterfrom
dhellmann:drop-available-method

Conversation

@dhellmann
Copy link
Copy Markdown
Member

@dhellmann dhellmann commented Dec 7, 2020 •
edited
Loading

It is up to the consumer, not the host, to decide when a host is ready
to have provisioning instructions added. The checks in the Available()
method will be inlined in cluster-api-provider-metal3.

Similarly, the host.Status.ErrorMessage field is the API for determining if a host has an error, so remove HasError().

@metal3-io-bot metal3-io-bot added 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 7, 2020
@dhellmann dhellmann mentioned this pull request Dec 7, 2020
Closed
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Dec 8, 2020

/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Dec 8, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@dhellmann dhellmann changed the title remove Available() method from BareMetalHost remove convenience methods from BareMetalHost Dec 8, 2020
@dhellmann dhellmann requested a review from zaneb December 8, 2020 14:21
@dhellmann
Copy link
Copy Markdown
Member Author

dhellmann commented Dec 8, 2020

/test-integration

@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 Dec 8, 2020
@s3rj1k s3rj1k mentioned this pull request Dec 9, 2020
Closed
@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Dec 10, 2020 •
edited
Loading

@dhellmann I am assuming that this PR should be merged as related metal3-io/cluster-api-provider-metal3#150 is merged already.

Some one needs to trigger test-integration for this PR.

@dhellmann
Copy link
Copy Markdown
Member Author

dhellmann commented Dec 10, 2020

@dhellmann I am assuming that this PR should be merged as related metal3-io/cluster-api-provider-metal3#150 is merged already.

Some one needs to trigger test-integration for this PR.

Yes, it should be safe to merge this now.

/test-integration

@dhellmann
remove Available() method from BareMetalHost
095bf63 
It is up to the consumer, not the host, to decide when a host is ready
to have provisioning instructions added. The checks in the Available()
method will be inlined in cluster-api-provider-metal3.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@dhellmann
remove HasError method from host type
29c784b 
Use the API field directly instead of the convenience method.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@dhellmann dhellmann force-pushed the drop-available-method branch from 60f4610 to dfa6cf4 Compare December 14, 2020 19:39
@dhellmann
Copy link
Copy Markdown
Member Author

dhellmann commented Dec 14, 2020

Rebased

/test-integration

@dhellmann
move error handling methods from host to private functions
b0c8c58 
Move public methods of the host type to private functions in the
controller package.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@dhellmann dhellmann force-pushed the drop-available-method branch from dfa6cf4 to b0c8c58 Compare December 14, 2020 19:43
@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Dec 15, 2020

@dhellmann I am curious, why not just make those methods unexported?

@maelk
Copy link
Copy Markdown
Member

maelk commented Dec 15, 2020

/test-integration
lgtm, not tagging to prevent a merge while a question is opened

@dhellmann
Copy link
Copy Markdown
Member Author

dhellmann commented Dec 15, 2020

@dhellmann I am curious, why not just make those methods unexported?

If they were private to the package where they are defined, then the controller couldn't call them.

@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Dec 16, 2020

@dhellmann Thanks, forgot about that controller is different package, sorry.

@zaneb Safe to merge.

@maelk
Copy link
Copy Markdown
Member

maelk commented Jan 11, 2021

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2021
@metal3-io-bot metal3-io-bot merged commit 93a6fd2 into metal3-io:master Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@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.

5 participants

@dhellmann @zaneb @metal3-io-bot @s3rj1k @maelk