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

Handling populate relavant values in machine pool #317

Merged
merged 1 commit into from
Oct 1, 2023
Merged

Handling populate relavant values in machine pool #317

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions provider/machinepool/machine_pool_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,14 +544,9 @@ func (r *MachinePoolResource) Update(ctx context.Context, req resource.UpdateReq

object := update.Body()

// update the autoscaling enabled with the plan value (important for nil and false cases)
state.AutoScalingEnabled = plan.AutoScalingEnabled
Copy link
Member

Choose a reason for hiding this comment

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

While AutoScalingEnabled can be not computed, I think Replicas need to be computed because when user doesn't declare value we put default =2
Anyway this should be done in populate and not here.

@JohnStrunk @bardielle @sagidayan what do you think?

I hope we will have here one convention how those cases should be handled

// update the Replicas with the plan value (important for nil and zero value cases)
state.Replicas = plan.Replicas

// Save the state:
r.populateState(object, state)
diags = resp.State.Set(ctx, state)
r.populateState(object, plan)
diags = resp.State.Set(ctx, plan)
Comment on lines +548 to +549
Copy link
Member

Choose a reason for hiding this comment

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

Good catch.

I think its a bit confusing that in some places we assign the Plan into parameter called state.
(for example in Create and populate functions)
We should refactor the code to make it more consistent and clear

resp.Diagnostics.Append(diags...)
}

Expand Down