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

add plugin for generic keycloak component #8826

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

fivetide
Copy link
Contributor

@fivetide fivetide commented Sep 3, 2024

SUMMARY

This MR adds a new plugin for generic management of keycloak components. It is derived from the keycloak_realm_keys plugin, but omits the key-specific handling of secrets and certs. Instead it is possible to manage any component.
Possible examples are ThemeSelector, LocaleSelector, UserStorageProvider.
see the official docs for more information:
https://www.keycloak.org/docs/latest/server_development/index.html#_providers

ISSUE TYPE
  • New Module/Plugin Pull Request
COMPONENT NAME

keycloak_component

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added module module new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit labels Sep 3, 2024
@ansibullbot
Copy link
Collaborator

The test botmeta failed with 1 error:

.github/BOTMETA.yml:0:0: Author fivetide not mentioned as active or inactive maintainer for plugins/modules/keycloak_component.py (mentioned are: eikef, ndclt, mattock, thomasbach-dev)

click here for bot help

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 3, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch labels Sep 4, 2024
Copy link
Collaborator

@felixfontein felixfontein 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 your contribution! Please reference to https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins for how to fix the BOTMETA issue (you need to add the module and yourself to .github/BOTMETA.yml).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the changelog fragment. New modules shouldn't have one (see https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins).

plugins/modules/keycloak_component.py Outdated Show resolved Hide resolved
plugins/modules/keycloak_component.py Outdated Show resolved Hide resolved
plugins/modules/keycloak_component.py Outdated Show resolved Hide resolved
plugins/modules/keycloak_component.py Outdated Show resolved Hide resolved
plugins/modules/keycloak_component.py Outdated Show resolved Hide resolved
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI new_contributor Help guide this first time contributor labels Sep 9, 2024
@ansibullbot ansibullbot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Sep 9, 2024
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Sep 9, 2024
Co-authored-by: Felix Fontein <[email protected]>
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Sep 10, 2024
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI and removed ci_verified Push fixes to PR branch to re-run CI labels Sep 10, 2024
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

hi @fivetide , thanks for your contribution!

Overall the code loogs good, I left just a couple of comments below, and as pointed before you need to add an entry for this module in the .github/BOTMETA.yml file.

@@ -0,0 +1,2 @@
minor_changes:
- keycloak_component - add plugin for managing any keycloak component (https://github.com/ansible-collections/community.general/pull/8826)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use punctuation here, this is later exported to public documentation.

Suggested change
- keycloak_component - add plugin for managing any keycloak component (https://github.com/ansible-collections/community.general/pull/8826)
- keycloak_component - add plugin for managing any keycloak component (https://github.com/ansible-collections/community.general/pull/8826).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file needs to be removed anyway.

Comment on lines +4 to +5
# Copyright (c) 2017, Eike Frost <[email protected]>
# Copyright (c) 2021, Christophe Gilles <[email protected]>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should update this ;-)

Comment on lines +23 to +24
used must have the requisite access rights. In a default Keycloak installation, admin-cli
and an admin user would work, as would a separate realm definition with the scope tailored
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using the markup here might be helpful for the readers:

Suggested change
used must have the requisite access rights. In a default Keycloak installation, admin-cli
and an admin user would work, as would a separate realm definition with the scope tailored
used must have the requisite access rights. In a default Keycloak installation, C(admin-cli)
and an C(admin) user would work, as would a separate realm definition with the scope tailored

required: true
config:
description:
- Dict specifying the key and its properties. Contents vary depending on the provider type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit odd to define the dict as being a dict. ;-) If I may suggest:

Suggested change
- Dict specifying the key and its properties. Contents vary depending on the provider type.
- Configuration properties for the provider.
- Contents vary depending on the provider type.

Or something along those lines.

Comment on lines +142 to +146
"""
Module execution

:return:
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO the code is cleaner without this.

Comment on lines +167 to +168
# Initialize the result object. Only "changed" seems to have special
# meaning for Ansible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit redundant to describe how module results look like. And, to be fair, diff also has special meaning for Ansible.

Suggested change
# Initialize the result object. Only "changed" seems to have special
# meaning for Ansible.
# Initialize the result object. Only "changed" seems to have special
# meaning for Ansible.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch check-before-release PR will be looked at again shortly before release and merged if possible. ci_verified Push fixes to PR branch to re-run CI module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_plugin New plugin plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants