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

Add new resource google_compute_target_ssl_proxy #569

Merged
merged 5 commits into from
Oct 13, 2017

Conversation

rosbo
Copy link
Contributor

@rosbo rosbo commented Oct 10, 2017

Fixes #62

=== RUN   TestAccComputeTargetSslProxy_import
--- PASS: TestAccComputeTargetSslProxy_import (66.96s)
=== RUN   TestAccComputeTargetSslProxy_basic
--- PASS: TestAccComputeTargetSslProxy_basic (66.86s)
=== RUN   TestAccComputeTargetSslProxy_update
=== RUN   TestAccComputeTargetSslProxy_update
--- PASS: TestAccComputeTargetSslProxy_update (110.85s)

@rosbo rosbo requested a review from danawillow October 10, 2017 23:25
@rosbo rosbo requested a review from selmanj October 11, 2017 21:41
return nil
}

func expandSslCertificates(configured []interface{}, d *schema.ResourceData, config *Config) ([]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function would be a bit simpler if it didn't take the configured parameter (which is of a very confusing type). Instead you can use the d parameter to extract out the ssl_certificates field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

d.Partial(true)

if d.HasChange("proxy_header") {
proxy_header := d.Get("proxy_header").(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to be that guy but this is using snake case rather than camel case.

It's totally readable to me but I imagine others might find it bad style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oupps, I let this one slip. Good catch!


if d.HasChange("proxy_header") {
proxy_header := d.Get("proxy_header").(string)
proxy_header_payload := &compute.TargetSslProxiesSetProxyHeaderRequest{
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if found.Name != rs.Primary.ID {
return fmt.Errorf("TargetSslProxy not found")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to do more testing here (e.g. that the stored parameters match the set parameters)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

foundCertsName = append(foundCertsName, GetResourceNameFromSelfLink(foundCert))
}

if !reflect.DeepEqual(foundCertsName, sslCerts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does order matter? If it doesn't, you might consider sorting or something before doing the deepEqual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, after waiting for the full test run to finish and reading the API docs, one and only one certificate must be specified at this time (they might support more in the future). So I added a MaxItems:1 on the schema for now and simplified this check.

From the docs:
"Currently exactly one SslCertificate resource must be specified."

@rosbo rosbo merged commit 66f876e into hashicorp:master Oct 13, 2017
@rosbo rosbo deleted the ssl-proxy branch October 13, 2017 18:02
negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
* Add target ssl proxy
* Add documentation
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
* Add target ssl proxy
* Add documentation
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
<!-- This change is generated by MagicModules. -->
/cc @rileykarson
@ghost
Copy link

ghost commented Mar 30, 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 👉 [email protected]. Thanks!

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

Successfully merging this pull request may close these issues.

Missing Google resource: Target SSL Proxy
2 participants