Skip to content

🌱 apply our own rules for host availability#150

Merged
metal3-io-bot merged 2 commits into
metal3-io:masterfrom
dhellmann:inline-availability-rules
Dec 10, 2020
Merged

🌱 apply our own rules for host availability#150
metal3-io-bot merged 2 commits into
metal3-io:masterfrom
dhellmann:inline-availability-rules

Conversation

@dhellmann
Copy link
Copy Markdown
Member

Do not rely on the host's Available() method to decide when a host can
be selected for provisioning.

Do not rely on the host's Available() method to decide when a host can
be selected for provisioning.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2020
@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 7, 2020
@dhellmann
Copy link
Copy Markdown
Member Author

/retest

@dhellmann
Copy link
Copy Markdown
Member Author

/test-integration

Copy link
Copy Markdown
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

Comment thread baremetal/metal3machine_manager.go Outdated
@metal3-io-bot
Copy link
Copy Markdown
Contributor

[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

We're trying to deprecate the convenience methods on API types.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@dhellmann dhellmann requested a review from zaneb December 8, 2020 14:15
@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Dec 8, 2020

@dhellmann what about ClearError() SetErrorMessage() ?

@dhellmann
Copy link
Copy Markdown
Member Author

@dhellmann what about ClearError() SetErrorMessage() ?

I added another patch to the series to remove those.

I think we can deal with the other public methods separately, to avoid making this PR gigantic.

@dhellmann
Copy link
Copy Markdown
Member Author

/test-integration

@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Dec 8, 2020

@dhellmann Do you intend to make a stable API promise?

I mean the only reason that I can think of removing those methods and make them private is to prepare a codebase for a new API version release.

@dhellmann
Copy link
Copy Markdown
Member Author

@dhellmann Do you intend to make a stable API promise?

I mean the only reason that I can think of removing those methods and make them private is to prepare a codebase for a new API version release.

I see this as part of continual cleanup. Those methods shouldn't have been implemented the way they are to begin with. I don't really think it's necessary to commit to making them a stable API.

We do try to keep other parts of the code stable, like github.com/metal3-io/baremetal-operator/pkg/bmc, but even in that case it's possible to address changes in the API when a dependency is updated in a consuming project.

Are you anticipating a specific issue from these changes? Or just concerned that we don't consider "internals" as needing to have stable APIs?

@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Dec 8, 2020

Are you anticipating a specific issue from these changes? Or just concerned that we don't consider "internals" as needing to have stable APIs?

Just concerned that some of the internals could actually be used by external projects, good example would be HasError() method.

On a side note not directly connected to this PR, can we have more frequent tag releases for code under metal3-io umbrella?
This should help with vendoring.

Copy link
Copy Markdown
Member

@maelk maelk left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2020
@metal3-io-bot metal3-io-bot merged commit a7c6536 into metal3-io:master Dec 10, 2020
honza pushed a commit to honza/cluster-api-provider-metal3 that referenced this pull request Jan 27, 2025
Fix BMO reboot api broken link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants