Handling populate relavant values in machine pool #317
Conversation
|
I think this is not the behavior we want. I'll take this offline 😉 . /hold |
|
This looks better 💪🏽 . Lets see CI |
| @@ -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 | |||
There was a problem hiding this comment.
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
| r.populateState(object, plan) | ||
| diags = resp.State.Set(ctx, plan) |
There was a problem hiding this comment.
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
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirarg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
No description provided.