added default realm, added password grant type #88
Conversation
There was a problem hiding this comment.
Thanks again for the PR! I only have a few small comments this time around.
After addressing these, I also want to look into writing some tests to ensure that the default realm (when provided) is respected when creating new resources and importing existing resources. To be honest, I am not quite sure how to approach this, but if you allow me to push changes to your fork, I can try to work on them this weekend.
keycloak/keycloak_client.go
Outdated
| httpClient := &http.Client{ | ||
| Timeout: time.Second * 5, | ||
| } | ||
| if defaultRealm != "" { | ||
| defaultRealm = realm |
There was a problem hiding this comment.
I don't think we should make this assumption here. The provider should only set a default realm for resources if the user explicitly set default_realm as a provider attribute instead of defaulting to the realm used for auth.
keycloak/keycloak_client.go
Outdated
| defaultRealm = realm | ||
| } | ||
| var clientCredentials *ClientCredentials | ||
| if clientId != "" { |
There was a problem hiding this comment.
This check shouldn't be needed since client_id is a required provider attribute.
| realmId = parts[0] | ||
| id = parts[1] | ||
| default: | ||
| return nil, fmt.Errorf("Resouce %s cannot be imported", d.Id()) |
There was a problem hiding this comment.
This error message isn't accurate, since this resource can be imported. This should be changed to tell the user the supported formats for tf import.
There was a problem hiding this comment.
This comment applies to a lot of other places in this PR as well.
|
@mrparkers fixed all your PR comments and added 2 pipelines to test different keycloak providers configs. Gave you an access to push to my fork. |
|
Removed |
| KEYCLOAK_URL: http://localhost:8080 | ||
| KEYCLOAK_REALM: master | ||
| KEYCLOAK_USER: keycloak | ||
| KEYCLOAK_PASSWORD: password |
There was a problem hiding this comment.
I like this idea. I'll have to update the branch protection to require these new statuses set by CircleCI.
| @@ -25,7 +26,7 @@ func resourceKeycloakCustomUserFederation() *schema.Resource { | |||
| }, | |||
| "realm_id": { | |||
| Type: schema.TypeString, | |||
| Required: true, | |||
| Optional: true, | |||
There was a problem hiding this comment.
Since default realm was removed, this should be changed back.
|
After the comment is addressed, I'll merge this and update the docs to explain the new provider attributes and auth type. Once that is done, I'll cut a release so you can start using this. Thanks again for the contribution! |
|
Oh, forgot to revert this change as well |
|
Everything is ready now |
|
@AndrewChubatiuk I'll try and get the docs updated in the next couple of days and cut a release for you. |
Restored #64 PR