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.

Sign up for GitHub

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

Jump to bottom

Add keycloak_default_groups resource for creating realm default groups #146

Merged
merged 10 commits into from
Sep 1, 2019
Merged

Add keycloak_default_groups resource for creating realm default groups #146

merged 10 commits into from
Sep 1, 2019

Conversation

fharding1
Copy link
Contributor

@fharding1 fharding1 commented Aug 30, 2019 •
edited
Loading

Summary

This PR adds a new keycloak_default_groups resource which allows you to manage default groups for a realm.

Closes #144
Closes #145

TODO

  • Validate that a group membership resource doesn't exist or figure out some other solution for that (or just document that using a default group and a group membership resource will lead to funky results), see Add support for default groups #144 (comment)
  • Add tests
  • Add ability to import default groups
  • Add documentation
  • Add an example

@fharding1 fharding1 changed the title Add keycloak_default_groups resource for creating realm default groups [WIP] Add keycloak_default_groups resource for creating realm default groups Aug 30, 2019
@fharding1
Copy link
Contributor Author

I changed the example to be formatted with the canonical terraform fmt. If this isn't desired, I can revert it.

@fharding1 fharding1 marked this pull request as ready for review August 30, 2019 20:02
@fharding1 fharding1 changed the title [WIP] Add keycloak_default_groups resource for creating realm default groups Add keycloak_default_groups resource for creating realm default groups Aug 30, 2019
@fharding1
Copy link
Contributor Author

Hmm, tests are failing on TestAccKeycloakDataSourceOpenidClient_basic and TestAccKeycloakDataSourceOpenidClientAuthorizationPolicy_basic, but I didn't touch those.

mrparkers
mrparkers reviewed Aug 31, 2019
View reviewed changes
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.

Yeah there are some flaky tests that I have a hard time reproducing locally, don't worry too much about those.

This PR looks great, nice work! I just had one nitpick comment but everything else looks great.

Also, I gave it some thought and I am not sure if I can come up with a way to actually validate the usage of keycloak_group_memberships along with default groups, so documenting it is good enough for me.

@fharding1
Properly check error in group memberships update
cd6417e 
See keycloak#144
Not part of the default groups feature but an unrelated bugfix.
Eventually this will be updated to use partial state rather than blow
up.
@fharding1
Use arrayOfStringsForTerraformResource rather than reimplementing it
ea6985f
@fharding1
Remove Also from keycloak_default_groups doc
efbac17
@fharding1
Copy link
Contributor Author

Had to force push to get rid of an accidentally commited update to the go.mod file (thanks go1.13rc2 😄 )

mrparkers
mrparkers approved these changes Sep 1, 2019
View reviewed changes
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.

thanks for the PR!

@mrparkers mrparkers merged commit e5b7538 into keycloak:master Sep 1, 2019
@fharding1
Copy link
Contributor Author

Thanks for the review and for merging! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrparkers mrparkers mrparkers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@fharding1 @mrparkers