Skip to content

Update disabling automated cleaning proposal#166

Merged
metal3-io-bot merged 1 commit intometal3-io:masterfrom
Nordix:update-proposal-151/feruz
Mar 17, 2021
Merged

Update disabling automated cleaning proposal#166
metal3-io-bot merged 1 commit intometal3-io:masterfrom
Nordix:update-proposal-151/feruz

Conversation

@fmuyassarov
Copy link
Copy Markdown
Member

@fmuyassarov fmuyassarov commented Mar 4, 2021

Update disabling automated cleaning proposal as per metal3-io/baremetal-operator#784 (comment)

@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 4, 2021
@fmuyassarov
Copy link
Copy Markdown
Member Author

fmuyassarov commented Mar 4, 2021

/cc @dhellmann @hardys

@fmuyassarov fmuyassarov mentioned this pull request Mar 4, 2021
Merged
@fmuyassarov fmuyassarov force-pushed the update-proposal-151/feruz branch from 7adca0c to 23b911e Compare March 4, 2021 12:41
@dhellmann
Copy link
Copy Markdown
Member

dhellmann commented Mar 4, 2021

This looks like what we discussed over on the code review. Thanks!

/approve

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2021
hardys
hardys reviewed Mar 4, 2021
View reviewed changes
Comment thread design/cluster-api-provider-metal3/allow_disabling_node_disk_cleaning.md Outdated
@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 4, 2021 •
edited
Loading

lgtm, just one minor nit re the choice of "None" vs perhaps "disabled/Disabled" - @dhellmann what are your thoughts on that?

Edit also the inspect annotation is e.g inspect.metal3.io: disabled - while they don't have to align, it'd probably be nice if the spec/annotations "disabled" value was consistent?

@fmuyassarov fmuyassarov force-pushed the update-proposal-151/feruz branch 4 times, most recently from de21ba5 to e43a05e Compare March 4, 2021 16:48
@dhellmann
Copy link
Copy Markdown
Member

dhellmann commented Mar 4, 2021

I'm happy with this draft, thanks @fmuyassarov. I'll leave it for @hardys to lgtm in case he has any other feedback.

/approve

@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 9, 2021

This looks good except for an unresolved comment from @dtantsur on the implementation PR metal3-io/baremetal-operator#784 (comment)

He mentions that "Full" could imply (or be used in future for) a full disk erase, which is not the current behavior (the disk metadata/partition-table is removed).

I don't have a strong opinion on what the alternative value should be (Metadata ?), but I wanted to ensure we have that discussion so that this PR matches the implementation.

@fmuyassarov fmuyassarov force-pushed the update-proposal-151/feruz branch from e43a05e to 2420ac8 Compare March 11, 2021 17:07
@fmuyassarov
Copy link
Copy Markdown
Member Author

fmuyassarov commented Mar 11, 2021

This looks good except for an unresolved comment from @dtantsur on the implementation PR metal3-io/baremetal-operator#784 (comment)

He mentions that "Full" could imply (or be used in future for) a full disk erase, which is not the current behavior (the disk metadata/partition-table is removed).

I don't have a strong opinion on what the alternative value should be (Metadata ?), but I wanted to ensure we have that discussion so that this PR matches the implementation.

replaced Full -> Metadata both in here and in the implementation.

@dhellmann
Copy link
Copy Markdown
Member

dhellmann commented Mar 11, 2021

Looks good.

/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

metal3-io-bot commented Mar 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, dtantsur, fmuyassarov

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

@hardys
Copy link
Copy Markdown
Member

hardys commented Mar 17, 2021

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2021
@metal3-io-bot metal3-io-bot merged commit 889729a into metal3-io:master Mar 17, 2021
@fmuyassarov fmuyassarov deleted the update-proposal-151/feruz branch June 27, 2022 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hardys hardys hardys left review comments

@dtantsur dtantsur dtantsur approved these changes

@dhellmann dhellmann Awaiting requested review from dhellmann

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fmuyassarov @dhellmann @hardys @metal3-io-bot @dtantsur