Fix keycloak_client_rolemapping role removal and diff #5619

flyingflo · 2022-11-28T16:11:40Z

SUMMARY

community.general.keycloak_client_rolemapping with state: absent removes all
roles. The intention is to only remove those listed.
The diff returned by the module has a before and after list. The latter contained the
changed roles, instead of the roles after the change.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

community.general.keycloak_client_rolemapping

ADDITIONAL INFORMATION

Fix these two issues.

ansibullbot · 2022-11-28T16:13:34Z

cc @Gaetan2907 @eikef @ndclt
click here for bot help

felixfontein

Thanks for your contribution! Can you please add a changelog fragment? Thanks.

plugins/module_utils/identity/keycloak/keycloak.py

plugins/modules/keycloak_client_rolemapping.py

flyingflo · 2022-11-29T16:29:17Z

Ok, I see, i have to work on unit tests..

changelogs/fragments/5619-keycloak-improvements.yml

Keycloak's delete_group_rolemapping API wrapper didn't pass data about the roles to remove to keycloak, resulting in removal of all roles. Follow the intended behaviour and delete only the roles listed in the module invocation. Signed-off-by: Florian Achleitner <flo@fopen.at>

The module's diff output wrongly showed the changed roles list as 'after' state. This is obviously wrong for role removal and also wrong for role addition, if there are other roles assigned. Use the result of the API query for 'end_state' for 'diff' as well. Signed-off-by: Florian Achleitner <flo@fopen.at>

Signed-off-by: Florian Achleitner <flo@fopen.at>

Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de>

felixfontein

As far as I can judge it it looks good. I will merge for the release on Tuesday if nobody objects.

patchback · 2022-12-05T05:23:04Z

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/f0b3bba030055183b864678208e187c24d2ca581/pr-5619

Backported as #5655

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* Keycloak: Fix client rolemapping removal Keycloak's delete_group_rolemapping API wrapper didn't pass data about the roles to remove to keycloak, resulting in removal of all roles. Follow the intended behaviour and delete only the roles listed in the module invocation. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Fix client_rolemapping diff The module's diff output wrongly showed the changed roles list as 'after' state. This is obviously wrong for role removal and also wrong for role addition, if there are other roles assigned. Use the result of the API query for 'end_state' for 'diff' as well. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Calculate client_rolemapping proposed state properly Signed-off-by: Florian Achleitner <flo@fopen.at> * Add changelog fragment Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de> * Fix for python2 unit test Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f0b3bba)

patchback · 2022-12-05T05:23:12Z

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/f0b3bba030055183b864678208e187c24d2ca581/pr-5619

Backported as #5656

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

felixfontein · 2022-12-05T05:23:18Z

@flyingflo thanks for your contribution!

* Keycloak: Fix client rolemapping removal Keycloak's delete_group_rolemapping API wrapper didn't pass data about the roles to remove to keycloak, resulting in removal of all roles. Follow the intended behaviour and delete only the roles listed in the module invocation. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Fix client_rolemapping diff The module's diff output wrongly showed the changed roles list as 'after' state. This is obviously wrong for role removal and also wrong for role addition, if there are other roles assigned. Use the result of the API query for 'end_state' for 'diff' as well. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Calculate client_rolemapping proposed state properly Signed-off-by: Florian Achleitner <flo@fopen.at> * Add changelog fragment Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de> * Fix for python2 unit test Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f0b3bba)

github-actions · 2022-12-05T05:40:12Z

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

…g role removal and diff (#5656) Fix keycloak_client_rolemapping role removal and diff (#5619) * Keycloak: Fix client rolemapping removal Keycloak's delete_group_rolemapping API wrapper didn't pass data about the roles to remove to keycloak, resulting in removal of all roles. Follow the intended behaviour and delete only the roles listed in the module invocation. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Fix client_rolemapping diff The module's diff output wrongly showed the changed roles list as 'after' state. This is obviously wrong for role removal and also wrong for role addition, if there are other roles assigned. Use the result of the API query for 'end_state' for 'diff' as well. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Calculate client_rolemapping proposed state properly Signed-off-by: Florian Achleitner <flo@fopen.at> * Add changelog fragment Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de> * Fix for python2 unit test Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f0b3bba) Co-authored-by: fachleitner <flo@fopen.at>

…g role removal and diff (#5655) Fix keycloak_client_rolemapping role removal and diff (#5619) * Keycloak: Fix client rolemapping removal Keycloak's delete_group_rolemapping API wrapper didn't pass data about the roles to remove to keycloak, resulting in removal of all roles. Follow the intended behaviour and delete only the roles listed in the module invocation. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Fix client_rolemapping diff The module's diff output wrongly showed the changed roles list as 'after' state. This is obviously wrong for role removal and also wrong for role addition, if there are other roles assigned. Use the result of the API query for 'end_state' for 'diff' as well. Signed-off-by: Florian Achleitner <flo@fopen.at> * Keycloak: Calculate client_rolemapping proposed state properly Signed-off-by: Florian Achleitner <flo@fopen.at> * Add changelog fragment Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de> * Fix for python2 unit test Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f0b3bba) Co-authored-by: fachleitner <flo@fopen.at>

ansibullbot added bug This issue/PR relates to a bug identity module module module_utils module_utils plugins plugin (any type) labels Nov 28, 2022

felixfontein reviewed Nov 28, 2022

View reviewed changes

plugins/module_utils/identity/keycloak/keycloak.py Show resolved Hide resolved

plugins/modules/keycloak_client_rolemapping.py Show resolved Hide resolved

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Nov 28, 2022

This comment was marked as outdated.

Sign in to view

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 29, 2022

flyingflo force-pushed the fix-keycloak-modules branch from cc194c3 to c9ac1cf Compare November 29, 2022 16:22

felixfontein reviewed Nov 29, 2022

View reviewed changes

changelogs/fragments/5619-keycloak-improvements.yml Outdated Show resolved Hide resolved

changelogs/fragments/5619-keycloak-improvements.yml Outdated Show resolved Hide resolved

flyingflo force-pushed the fix-keycloak-modules branch from c9ac1cf to 7320bd4 Compare November 30, 2022 08:18

flyingflo and others added 4 commits November 30, 2022 09:26

Keycloak: Calculate client_rolemapping proposed state properly

c79bf25

Signed-off-by: Florian Achleitner <flo@fopen.at>

Add changelog fragment

2801b70

Signed-off-by: Florian Achleitner <flo@fopen.at> Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de>

flyingflo force-pushed the fix-keycloak-modules branch from bf59c8a to 2801b70 Compare November 30, 2022 08:27

Fix for python2 unit test

c4c5e73

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 30, 2022

felixfontein approved these changes Dec 3, 2022

View reviewed changes

felixfontein merged commit f0b3bba into ansible-collections:main Dec 5, 2022

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Dec 5, 2022

patchback bot mentioned this pull request Dec 5, 2022

[PR #5619/f0b3bba0 backport][stable-5] Fix keycloak_client_rolemapping role removal and diff #5655

Merged

patchback bot mentioned this pull request Dec 5, 2022

[PR #5619/f0b3bba0 backport][stable-6] Fix keycloak_client_rolemapping role removal and diff #5656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix keycloak_client_rolemapping role removal and diff #5619

Fix keycloak_client_rolemapping role removal and diff #5619

flyingflo commented Nov 28, 2022

ansibullbot commented Nov 28, 2022

felixfontein left a comment

This comment was marked as outdated.

flyingflo commented Nov 29, 2022

felixfontein left a comment

patchback bot commented Dec 5, 2022 •

edited

Loading

patchback bot commented Dec 5, 2022 •

edited

Loading

felixfontein commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

Fix keycloak_client_rolemapping role removal and diff #5619

Fix keycloak_client_rolemapping role removal and diff #5619

Conversation

flyingflo commented Nov 28, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Nov 28, 2022

felixfontein left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

flyingflo commented Nov 29, 2022

felixfontein left a comment

Choose a reason for hiding this comment

patchback bot commented Dec 5, 2022 • edited Loading

Backport to stable-5: 💚 backport PR created

patchback bot commented Dec 5, 2022 • edited Loading

Backport to stable-6: 💚 backport PR created

felixfontein commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

Docs Build 📝

patchback bot commented Dec 5, 2022 •

edited

Loading

patchback bot commented Dec 5, 2022 •

edited

Loading