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

Clarify realm id / name and expose realm id as new realm attribute #270

Merged
merged 1 commit into from
May 21, 2020

Conversation

dmeyerholt
Copy link
Contributor

@dmeyerholt dmeyerholt commented Apr 8, 2020

Coming from discussions here #216 my shot at clarifying usage of realm name and id.
Main points are:

  • the requests to the API require the usage of the realm name, not the id. this is a rather cosmetic change in keycloak/realm.go
  • in the realm resource, now the "real" realm id is exposed as "internal_id" attribute
  • we retain the default behaviour of creating a realm with realmId == realmName so nothing breaks new/existing realms created by the provider
  • This offers the possibility to import existing realms which have a realm id that differs from the realm's name. This is relevant for possible keycloak component which use the realm id for the parent_id attributes. Additional PR coming for the custom_user_federation if this PR gets through. I think, the ldap stuff is also affected by possible id and name mismatches (through the use of parent_id attributes)

@dmeyerholt dmeyerholt mentioned this pull request Apr 8, 2020
@dmeyerholt dmeyerholt changed the title Clearify realm id / name and expose realm id as new realm attribute Clarify realm id / name and expose realm id as new realm attribute Apr 8, 2020
@mrparkers
Copy link
Contributor

This looks good so far. Here are a few changes I'd also like to see before I'm ready to merge this:

  • Add a test that creates a keycloak_realm and asserts that its internal_id attribute is not the same as its name.
  • Add this property to the keycloak_realm data source.
  • Document this new attributes on the doc pages for the keycloak_realm resource and data source.

Let me know if you need help with any of these things.

Thanks!

@dmeyerholt dmeyerholt force-pushed the clearify-realm-id-and-name branch from 827cfa0 to 3cec8e6 Compare April 14, 2020 18:29
@dmeyerholt dmeyerholt force-pushed the clearify-realm-id-and-name branch 4 times, most recently from 1cbfa76 to fe24d8c Compare May 11, 2020 12:58
@dmeyerholt
Copy link
Contributor Author

hey @mrparkers ,
had time to finish this pr with your suggestions. Skipped documentation of data source, as that doc references the realm doc for all supported attributes.
Hope you are fine with this now.
will start new pr for improving the custom_userfederation resource to leverage this new internalId attribute.

@dmeyerholt dmeyerholt marked this pull request as ready for review May 11, 2020 13:09
@dmeyerholt dmeyerholt force-pushed the clearify-realm-id-and-name branch 4 times, most recently from 305fad4 to af6acde Compare May 12, 2020 08:31
@dmeyerholt dmeyerholt force-pushed the clearify-realm-id-and-name branch from af6acde to 23464e7 Compare May 20, 2020 18:10
Copy link
Contributor

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants