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: allow subnet link lookup via field, project and region #591

Conversation

gsdevme
Copy link

@gsdevme gsdevme commented Oct 16, 2017

resource_compute_forwarding_rule doesn't support a string configuration, its not really ideal as Id like to have the network layer of Terraform separate.

I've functionally tested this and its working. I feel like long term getSubnetworkLink might need refactoring because it assumes Zonal configuration to get the region rather than just having region as an argument.

It seems like getSubnetworkLink should actually be called something else and more specific to its use case.. for now though Ive done this however

Without this change you get the following error.

google_compute_forwarding_rule.default: Error creating ForwardingRule: googleapi: Error 400: Invalid value for field 'resource.subnetwork': 'xxxx'. The URL is malformed., invalid

@gsdevme
Copy link
Author

gsdevme commented Oct 20, 2017

Hi, is there anything I can do to speed up the merging? any suggestions?

@danawillow
Copy link
Contributor

Hey @gsdevme, we try to get to all PRs within a week and we're a bit shortstaffed this week, so I just ask for your patience. That being said, code reviews are #2 on my to-do list for today so I suspect I'll review it this afternoon :)

@gsdevme
Copy link
Author

gsdevme commented Oct 20, 2017 via email

@danawillow
Copy link
Contributor

Hey @gsdevme, thanks for the PR! We actually are just starting to standardize on how we allow fields to have either a name or a self_link value. Take a look at https://github.com/terraform-providers/terraform-provider-google/blob/master/google/field_helpers.go and it's usage (ParseNetworkFieldValue is already being used for network within resource_compute_forwarding_rule.go). You'd need to add a new one for regionFieldValues, but it would be a very welcome addition to the code base!

If that seems like it'll be too much work, we can check this in as-is, but it'll probably get overridden pretty soon. Either way I'd like to see a test (we actually don't have any that set a subnetwork right now, so it would be great to have one for setting by name and one for setting by URL)

@nat-henderson
Copy link
Contributor

I've just checked, and as @danawillow said, this no longer needs to be merged. Thanks for the PR, though!

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
…FQDNs (hashicorp#591)

Signed-off-by: Modular Magician <magic-modules@google.com>
@ghost
Copy link

ghost commented Mar 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants