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

This feature enables support for the L4 ILB Flex Ports feature currently available in alpha. #1859

Closed

Conversation

modular-magician
Copy link
Collaborator

/cc @craigatgoogle

@modular-magician modular-magician force-pushed the codegen-pr-396 branch 2 times, most recently from 57e5940 to 1fad59f Compare August 17, 2018 20:11
@danawillow
Copy link
Contributor

@craigatgoogle, do you know whether the feature is in public alpha or whether the user has to be whitelisted? I don't love that none of our tests are going to cover this, but if it needs a whitelist then we'll need to get HashiCorp on the whitelist.

@craigdbarber
Copy link

Verified with the feature team that allPorts is available in public alpha. Added an acctest for the allPorts field, and in the process added a test template to MM for the ForwardingRule tests.

@danawillow
Copy link
Contributor

Hey @craigatgoogle, this is what I got when running the new test:

------- Stdout: -------
=== RUN   TestAccComputeForwardingRule_allPorts
--- FAIL: TestAccComputeForwardingRule_allPorts (83.71s)
	testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
		
		* google_compute_forwarding_rule.foobar: 1 error(s) occurred:
		
		* google_compute_forwarding_rule.foobar: Error creating ForwardingRule: googleapi: Error 400: Invalid value for field 'resource.ports[0]': ''. Forwarding rules with backend services must specify at least one port., invalid
FAIL

@craigdbarber
Copy link

Spoke with @danawillow offline about the test issue, turned out to be due to the terraform generation script missing a section for resources utilizing the alpha APIs. Just pushed a fix for this to MM. An update to this PR should be inbound from Magician.

@@ -221,19 +221,14 @@ func testAccCheckComputeAutoscalerUpdated(n string, max int64) resource.TestChec

func testAccComputeAutoscaler_scaffolding(it_name, tp_name, igm_name string) string {
return fmt.Sprintf(`
data "google_compute_image" "my_image" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no, looks like a different PR in terraform modified tests that get copied over and so this is trying to overwrite them. Would you mind just doing these changes too in your PR so that we don't have to worry about merges and stuff?

Choose a reason for hiding this comment

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

Happy to, although we should figure out who wrote that PR and let them know they should be doing those changes in MM for this resource going forward :)

@ghost ghost added size/m and removed size/l labels Aug 27, 2018
@rileykarson rileykarson requested a review from emilymye August 27, 2018 21:31
@craigdbarber
Copy link

@emilymye with @danawillow OOO this week, would you mind finishing this review?

@emilymye
Copy link
Contributor

emilymye commented Aug 27, 2018

It seems like this is the first alpha resource, and I have questions. The way the current provider works (i.e how URLs are decided), any users that were using forwarding rules will suddenly not be able to use their existing, non-alpha resources without asking to be whitelisted for the alpha APIs or downgrading Terraform. Is this what we expect, and if so...that seems non ideal. I don't think the alpha APIs are that easy to enable for users, but correct me if I am wrong. (cc @paddycarver and @danawillow @ndmckinley for when they are back)

The reason I ask this is because I actually don't have access to the alpha API in my current setup. Can you comment with the test result from your new test here?

Finally, this is less of a concern right now, but I'm a little worried about not adding send_empty_value: true in the api.yaml for MM. Right now it isn't an issue because ForceNew, but in the future, we might run into errors where setting to empty after it was set to true creates no-ops/permadiff hell.

@craigdbarber
Copy link

Here's the test results:

$ make testacc TESTARGS="--run TestAccComputeForwardingRule_allPorts"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v --run TestAccComputeForwardingRule_allPorts -timeout 120m
? github.com/terraform-providers/terraform-provider-google [no test files]
=== RUN TestAccComputeForwardingRule_allPorts
--- PASS: TestAccComputeForwardingRule_allPorts (117.02s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 117.255s
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-google/scripts 0.154s [no tests to run]

@emilymye
Copy link
Contributor

So it turns out I didn't have the alpha SDK installed, which apparently also flips the flag for alpha compute. However, based on our offline discussion, @craigatgoogle is just going to wait for beta support. I'll approve for now but DO NOT MERGE until beta comes out.

@rileykarson
Copy link
Collaborator

Oh this is done! It went into beta. hashicorp/terraform-provider-google-beta#297

@rileykarson rileykarson closed this Jan 4, 2019
@ghost
Copy link

ghost commented Feb 3, 2019

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 Feb 3, 2019
@modular-magician modular-magician deleted the codegen-pr-396 branch November 16, 2024 20:46
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.

7 participants