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

Add mappings for the SAML type connection #118

Merged
merged 11 commits into from
Jul 1, 2020
Merged

Add mappings for the SAML type connection #118

merged 11 commits into from
Jul 1, 2020

Conversation

kgunbin
Copy link
Contributor

@kgunbin kgunbin commented Jun 18, 2020

This PR adds the options for the SAML connection type.

  • Added attributes supported by the Management API V2
  • Added relevant tests

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.

Just a few minor suggestions. Otherwise, I think this looks good and it is certainly needed.

Thanks!

management/connection.go Outdated Show resolved Hide resolved
management/connection.go Outdated Show resolved Hide resolved
management/connection.go Show resolved Hide resolved
management/connection.go Outdated Show resolved Hide resolved
kgunbin and others added 5 commits June 26, 2020 11:55
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>
Copy link
Contributor

@alexkappa alexkappa left a comment

Choose a reason for hiding this comment

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

Thank you for this @kgunbin!

I made some suggestions with regards to naming and casing, otherwise I think it looks quite alright to me.

management/connection.go Outdated Show resolved Hide resolved
management/connection.go Outdated Show resolved Hide resolved
management/connection.go Outdated Show resolved Hide resolved
management/connection_test.go Outdated Show resolved Hide resolved
management/connection_test.go Outdated Show resolved Hide resolved
@@ -227,7 +227,7 @@ func (m *Management) request(method, uri string, v interface{}) error {
return newError(res.Body)
}

if res.StatusCode != http.StatusNoContent {
if res.StatusCode != http.StatusNoContent && res.StatusCode != http.StatusAccepted {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you give some context over this check? What caused a response of status accepted? Just curious..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests were failing on the deletion step, and according to https://auth0.com/docs/api/management/v2#!/Connections/delete_connections_by_id it is expected that DELETE can respond with 202 instead of 204
I assume that would better come as a separate PR, but without this change I was unable to get tests green for alexkappa/terraform-provider-auth0#244

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, I missed that! Thanks! And no worries, we can keep it in with this PR.

@alexkappa alexkappa merged commit 2efe033 into go-auth0:master Jul 1, 2020
@alexkappa
Copy link
Contributor

Many thanks for working on this @kgunbin!

@alexkappa
Copy link
Contributor

I've cut v4.4.0 if you would like to update alexkappa/terraform-provider-auth0#244.

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