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

fix!: Remove unnecessary breaking changes when upgrading to v11.0.0+ #417

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

thatcoleyouknow
Copy link

PR #330 introduced a breaking change for existing templates that were deployed with this module. For some consumers, this creates an unnecessary redeployment of all downstream instances that may come at great costs. This is an optional argument and I feel this should be treated as such with a dynamic block. This PR enables consumers that are upgrading from version <10.0.0 to do so without recreating their template and downstream instances while also allowing consumers that are using version >10.0.0 to avoid recreating their instances and templates by simply setting the argument to match their preferred value.

Note: I made all of these changes in the GitHub editor, not my IDE, so if I missed something, please let me know and I'll get it fixed.

@thatcoleyouknow thatcoleyouknow requested a review from a team as a code owner July 29, 2024 22:22
@thatcoleyouknow thatcoleyouknow changed the title Remove unnecessary breaking change with total_egress_bandwidth_tier argument of instance_template submodule fix!: Remove unnecessary breaking change with total_egress_bandwidth_tier argument of instance_template submodule Jul 29, 2024
@daniel-cit
Copy link

/gcbrun

@thatcoleyouknow
Copy link
Author

thatcoleyouknow commented Aug 3, 2024

@daniel-cit - My latest commits create another breaking change that reverts two breaking changes made in #357. Those two breaking changes include:

  1. Changing the google_compute_instance_template resource to use the google-beta provider
  2. Adding the maintenance_interval argument on the google_compute_instance_template resource

I understand the need to be able to support beta features and how this PR will affect consumers that are already using version >11.0.0. This repo should provide that functionality but without forcing consumers to recreate existing resources when beta features like this are added. I feel beta features, especially those that frequently introduce breaking changes, should be handled in a beta submodule, like the GKE team does with these modules: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/modules. I would be happy to add a beta-instance-template submodule to my PR if you all would like to see it and think through it. If it made more sense, we could make this the beta module and create another that uses the GA provider to avoid a negative experience for those that are using version > 11.0.0.

Do you or the broader team have any thoughts on this?

@apeabody
Copy link
Contributor

apeabody commented Aug 6, 2024

/gcbrun

@thatcoleyouknow thatcoleyouknow changed the title fix!: Remove unnecessary breaking change with total_egress_bandwidth_tier argument of instance_template submodule fix!: Remove unnecessary breaking changes when upgrading to v11.0.0+ Aug 8, 2024
@apeabody
Copy link
Contributor

apeabody commented Aug 8, 2024

@daniel-cit - My latest commits create another breaking change that reverts two breaking changes made in #357. Those two breaking changes include:

  1. Changing the google_compute_instance_template resource to use the google-beta provider
  2. Adding the maintenance_interval argument on the google_compute_instance_template resource

I understand the need to be able to support beta features and how this PR will affect consumers that are already using version >11.0.0. This repo should provide that functionality but without forcing consumers to recreate existing resources when beta features like this are added. I feel beta features, especially those that frequently introduce breaking changes, should be handled in a beta submodule, like the GKE team does with these modules: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/modules. I would be happy to add a beta-instance-template submodule to my PR if you all would like to see it and think through it. If it made more sense, we could make this the beta module and create another that uses the GA provider to avoid a negative experience for those that are using version > 11.0.0.

Do you or the broader team have any thoughts on this?

Thanks for the contribution @thatcoleyouknow!

My initial suggestion would be to split this into two PRs, with one focused solely on the provider change. The reason being is the other changes such as dynamic network_performance_config make perfect sense and can be reviewed/committed quickly, regardless of the sub-module's provider strategy. Thanks!

@thatcoleyouknow
Copy link
Author

@apeabody - Before I invest time into splitting this out into two modules, do you, as a codeowner, agree with that approach I suggested above? If not, do you care to elaborate on how you would like to see beta features supported in a single module?

@apeabody
Copy link
Contributor

apeabody commented Aug 9, 2024

@apeabody - Before I invest time into splitting this out into two modules, do you, as a codeowner, agree with that approach I suggested above? If not, do you care to elaborate on how you would like to see beta features supported in a single module?

Hi @thatcoleyouknow - I'm open to splitting into two modules, however similar to terraform-google-kubernetes-engine, to minimize maintenance and limit drift and need for extensive testing, we should also implement using a single autogen source that creates both. That way we only need to gate the beta specific features and provider, and otherwise the modules will be identical. Happy to assist on the autogen as I know this is a larger project than probably envisioned.

-Ref: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/autogen/main
-Ref: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/autogen_modules.json

Copy link

github-actions bot commented Oct 8, 2024

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Stale label Oct 8, 2024
@thatcoleyouknow
Copy link
Author

I still plan to do this. Bumping the thread to keep it open

@apeabody apeabody removed the Stale label Oct 14, 2024
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.

3 participants