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.

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

feat: remove explicit properties enabling specific cs provisioner steps #927

Closed
wants to merge 1 commit into from

Conversation

miguelsorianod
Copy link
Collaborator

Now the steps are enabled by default, and the properties that enable/disable some of them have been renamed.

What this PR does

Jira:
Link to demo recording:

Special notes for your reviewer

@miguelsorianod
Copy link
Collaborator Author

This MR can only be merged after the corresponding MR in CS has been merged and rolled out

Copy link
Collaborator

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

Requested a change just to leave the code in a slightly cleaner state.

"np_provisioner_deprovision_enabled": "true",
}
additionalProperties := map[string]string{}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest just deleting this whole function and declaring a map[string]string{} where getDefaultAdditionalProperities is called.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fine for me if you want. The reason I kept the function is to have it there in case in the future we want to add default properties, to have it contained in that function.

Let me know if you still prefer what you suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood. If there's a need in the future for always-present additional properties we can reinstate that function, but I don't foresee it. So I'd still prefer it removed, please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 I will change it then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@miguelsorianod miguelsorianod force-pushed the update-cs-provisionerannotations-state branch from 6a8755a to afba808 Compare December 4, 2024 16:52
Now the steps are enabled by default, and the properties that
enable/disable some of them have been renamed
@miguelsorianod miguelsorianod force-pushed the update-cs-provisionerannotations-state branch from afba808 to 5dd6faf Compare December 4, 2024 17:05
@@ -42,7 +42,7 @@ func NewMockClusterServiceClient() MockClusterServiceClient {
func (mcsc *MockClusterServiceClient) GetConn() *sdk.Connection { panic("GetConn not implemented") }

func (csc *MockClusterServiceClient) AddProperties(builder *cmv1.ClusterBuilder) *cmv1.ClusterBuilder {
additionalProperties := getDefaultAdditionalProperities()
additionalProperties := map[string]string{}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mbarnes I also changed it here

Copy link
Collaborator

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@miguelsorianod
Copy link
Collaborator Author

We are going to rollout the changes here together with CS Image update in #961. I am closing this issue and we moved the commits in here to the other MR.

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.

2 participants