Skip to content
This repository has been archived by the owner on Mar 8, 2022. It is now read-only.

Add SAML options to auth0_connection resource #244

Merged
merged 12 commits into from
Jul 1, 2020
Merged

Add SAML options to auth0_connection resource #244

merged 12 commits into from
Jul 1, 2020

Conversation

kgunbin
Copy link
Contributor

@kgunbin kgunbin commented Jun 18, 2020

Based on go-auth0/auth0#118

Proposed Changes

  • Add SAML options to auth0_connection resource. Only support the options that are both readable and writable.
  • Added an acceptance test and documentation

Acceptance Test Output

$ make testacc TESTS=TestAccConnectionSAML
==> Checking that code complies with gofmt requirements...
?   	github.com/terraform-providers/terraform-provider-auth0	[no test files]
=== RUN   TestAccConnectionSAML
--- PASS: TestAccConnectionSAML (1.82s)
PASS
coverage: 10.8% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0	1.845s	coverage: 10.8% of statements
?   	github.com/terraform-providers/terraform-provider-auth0/auth0/internal/debug	[no test files]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0/internal/random	0.023s	coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok  	github.com/terraform-providers/terraform-provider-auth0/auth0/internal/validation	0.009s	coverage: 0.0% of statements [no tests to run]
?   	github.com/terraform-providers/terraform-provider-auth0/version	[no test files]

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

kgunbin added 2 commits June 18, 2020 14:10
Only support the options that are both readable and writable.
Added an acceptance test and documentation
Copy link
Contributor

@jbauth0 jbauth0 left a comment

Choose a reason for hiding this comment

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

Suggestions based on my suggestions to the go library changes.

I didn't test these - I just typed them in. Hopefully, I got it mostly right.

Thanks again for making the PR.

auth0/resource_auth0_connection.go Outdated Show resolved Hide resolved
auth0/structure_auth0_connection.go Outdated Show resolved Hide resolved
auth0/structure_auth0_connection.go Outdated Show resolved Hide resolved
website/docs/r/connection.html.md Outdated Show resolved Hide resolved
website/docs/r/connection.html.md Show resolved Hide resolved
auth0/resource_auth0_connection.go Show resolved Hide resolved
auth0/resource_auth0_connection.go Show resolved Hide resolved
kgunbin and others added 8 commits June 26, 2020 12:37
Co-authored-by: jbauth0 <49204697+jbauth0@users.noreply.github.com>
Co-authored-by: jbauth0 <49204697+jbauth0@users.noreply.github.com>
Co-authored-by: jbauth0 <49204697+jbauth0@users.noreply.github.com>
Co-authored-by: jbauth0 <49204697+jbauth0@users.noreply.github.com>
Co-authored-by: jbauth0 <49204697+jbauth0@users.noreply.github.com>
Co-authored-by: jbauth0 <49204697+jbauth0@users.noreply.github.com>
Co-authored-by: jbauth0 <49204697+jbauth0@users.noreply.github.com>
"signing_cert": o.GetSigningCert(),
"protocol_binding": o.GetProtocolBinding(),
"debug": o.GetDebug(),
"idpinitiated": o.GetIdpInitiated(),
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be flattened as well?

ClientID: String(d, "client_id"),
ClientProtocol: String(d, "client_protocol"),
ClientAuthorizequery: String(d, "client_authorizequery"),
},
Copy link
Owner

Choose a reason for hiding this comment

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

I suspect there is an error here. String(d, "client_id") should be run inside a Set(d, "idpinitiated").Elem(...) block.

}
}
}
`
Copy link
Owner

Choose a reason for hiding this comment

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

Could we add a test for updates? Some common issues tend to surface which get caught with update tests. Also please test the idpinitiated block.

"urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST",
}, true),
},
"idpinititated": {
Copy link
Owner

Choose a reason for hiding this comment

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

We could make this a little more readable:

Suggested change
"idpinititated": {
"idp_inititated": {

Type: schema.TypeString,
Optional: true,
},
"client_authorizequery": {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"client_authorizequery": {
"client_authorize_query": {

@kgunbin kgunbin marked this pull request as ready for review July 1, 2020 08:32
@alexkappa alexkappa merged commit 0037b4d into alexkappa:master Jul 1, 2020
@alexkappa
Copy link
Owner

Awesome work @kgunbin thank you for this! Watch out for a new release soon.

@jbauth0
Copy link
Contributor

jbauth0 commented Jul 8, 2020

Yes - thank you for all your work @kgunbin!

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.

3 participants