fix: nodepool autoscaling vars avail in GKE 1.24.1 result in conflicts. Preserve default behavior

[Aurelian-Shuttleworth]

Changes introduced in feat: add nodepool autoscaling vars avail in GKE 1.24.1 result in conflicts when using total limits.

Currently, google_container_node_pool resource states that total and per zone limits can't be used together.

To address this issue and maintain the current behaviour where min and max values for a node pool are set by default, this fix strays from standard behaviour where the value is set to null. Please advise if this is acceptable.
If this is not, I have prepared a second fix where the values are nulled. fix: nodepool autoscaling vars avail in GKE 1.24.1 result in conflicts. Change default behavior to null

[apeabody]

/gcbrun

[Aurelian-Shuttleworth]

@apeabody Can you share the results of the terraform-google-kubernetes-engine-int-trigger test?

thank you ^_^

[apeabody]

@apeabody Can you share the results of the terraform-google-kubernetes-engine-int-trigger test?

thank you ^_^

It was a timeout, I re-triggered as it could be transient.

[ericyz]

/gcbrun

[ericyz]

/gcbrun

[Aurelian-Shuttleworth]

@ericyz can you check if the check failed due to timeout again or if an issue has poped up?

Thank you.

[ericyz]

/gcbrun

[ericyz]

It timed out. I re-triggered it and will post any error logs.

[ericyz]

/gcbrun

[Aurelian-Shuttleworth]

@ericyz and @apeabody, can either of you advise me on how I should handle rebases more effectively?

I have noticed every time I need to rebase on the default branch, /gcbrun needs to be run and often, due to timeouts and permissions, a new rebase needed by the time /gcbrun can return a pass.

Please let me know if I can do anything to make the PR process easier for both of you ^_^

[apeabody]

/gcbrun

[ericyz]

/gcbrun

[ericyz]

/gcbrun

Sorry, looked at wrong PR and will approve after review

[ericyz]

/gcbrun

[ericyz]

/gcbrun

[ericyz]

Hi @Aurelian-Shuttleworth , thanks for your contribution.

[ericyz]

/gcbrun

[ericyz]

@Aurelian-Shuttleworth - I prefer to setting to null like the second fix you proposed. We would like to minimise the logic handling the conflict properties and surface the error when users set both min_count and total_min_count.

@bharathkkb - We can include this change as an break change. Do you see other implications on this?

[ericyz]

/gcbrun

[Aurelian-Shuttleworth]

@ericyz I figured that would be the presence ^_^

I will look at the tests and what can be updated in my alternative PR.

[Aurelian-Shuttleworth]

@ericyz Having a look around the test suite, I can only test changes by spinning it up, and I can't afford the cloud costs at the moment.

I believe all that needs to be done to allow my other PR to succeed is to add the node pool settings that would otherwise be set by the default variable values.

Can one of the code maintainers add those values and confirm?

[ericyz]

Hi @Aurelian-Shuttleworth , your understanding is correct, but please hold for a moment. I will confirm the direction and get back to you

[ericyz]

LGTM

[ericyz]

Hi @Aurelian-Shuttleworth , please raise another PR for your alternative suggested solution if you are alright. We will include it in the next breaking release. I can help you on the test cases

[ericyz]

@apeabody - could you please peer review and merged when ready? Thank you

[apeabody]

@ericyz and @apeabody, can either of you advise me on how I should handle rebases more effectively?

I have noticed every time I need to rebase on the default branch, /gcbrun needs to be run and often, due to timeouts and permissions, a new rebase needed by the time /gcbrun can return a pass.

Please let me know if I can do anything to make the PR process easier for both of you ^_^

Thanks @Aurelian-Shuttleworth - Unfortunately at this time one of us will need to add a /gcbrun after each commit to trigger the CI.

[apeabody]

Thanks for the contribution @Aurelian-Shuttleworth!

[apeabody]

The INT failure appears unrelated, but we'll need to get it to clear.