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!: Adds support for labels #96

Merged
merged 10 commits into from
Jun 27, 2022

Conversation

willroberts
Copy link
Contributor

No description provided.

@comment-bot-dev
Copy link

comment-bot-dev commented May 20, 2022

Thanks for the PR! 🚀
✅ Lint checks have passed.

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @willroberts

versions.tf Show resolved Hide resolved
@bharathkkb bharathkkb changed the title Adds support for labels feat!: Adds support for labels Jun 16, 2022
main.tf Outdated
@@ -28,6 +28,7 @@ data "google_compute_subnetwork" "network" {
}

resource "google_compute_forwarding_rule" "default" {
provider = google-beta
Copy link
Member

Choose a reason for hiding this comment

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

I was doing some more digging and it seemed to me like the regular provider may actually support this but it is not documented. Did you try this with the regular provider before using beta?
https://github.com/hashicorp/terraform-provider-google/blob/922b67a420efa3bfabde02dd097ef1e1d68d7316/google/resource_compute_forwarding_rule.go#L289

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested and confirmed that forwarding rules support labels without the beta provider, so I've removed the provider attribute from that resource. These docs may be slightly out of date: https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_forwarding_rule#labels

The health checks do still require the beta provider, so I'll keep it in versions.tf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are currently using provider version >= 3.53, < 5.0, which does not contain label support outside of beta. Should we bump our version (to >= 4.26, < 5.0), or leave the beta provider on the forwarding rule resource for now?

Copy link
Member

Choose a reason for hiding this comment

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

@willroberts Lets bump the version to 4.26

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @willroberts - It looks like you will need to (at least) also bump

to 3.1 to support the TPG4 requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still looking at this; the instance_template dependency also needed to be updated, and I'm tracking down some additional sources of provider < 4.0.0 constraints.

The terraform_validate steps in make docker_test_lint appear to be installing provider 3.62.0 and beta provider 3.90.1.

Copy link
Contributor Author

@willroberts willroberts Jun 23, 2022

Choose a reason for hiding this comment

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

Got this working locally. /usr/local/bin/terraform_validate in the container uses terraform init -backend=false. Adding -upgrade to the arguments resolved the dependencies successfully.

Looks like it's also passing in CI now.

@bharathkkb bharathkkb merged commit b944015 into terraform-google-modules:master Jun 27, 2022
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.

4 participants