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

keycloak_client: Refactor & fix validate-modules issues #52438

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@dagwieers
Copy link
Member

dagwieers commented Feb 18, 2019

SUMMARY

This PR includes:

  • Adding parameter types
  • Fix validate-modules issue
  • Improve parameter types and resulting changes
  • Fix check-mode and return values

This PR needs to be verified and tested by maintainer(s).

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME

keycloak_client

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 18, 2019

@samdoran samdoran removed the needs_triage label Feb 19, 2019

@gundalow gundalow changed the title keycloak_client: Fix validate-modules issues keycloak_client: Refactor & fix validate-modules issues Feb 22, 2019

@gundalow
Copy link
Contributor

gundalow left a comment

I've updated the PR title to make it clearer this is a larger change

@@ -400,7 +400,6 @@ lib/ansible/modules/files/synchronize.py E324
lib/ansible/modules/files/synchronize.py E327
lib/ansible/modules/files/unarchive.py E323
lib/ansible/modules/identity/cyberark/cyberark_user.py E324
lib/ansible/modules/identity/keycloak/keycloak_client.py E324
lib/ansible/modules/identity/keycloak/keycloak_clienttemplate.py E324

This comment has been minimized.

@gundalow

gundalow Feb 22, 2019

Contributor

Any reason E324 (defaults != argspec) hasn't been fixed?

This comment has been minimized.

@dagwieers

dagwieers Feb 22, 2019

Author Member

Isn't that line saying that we did ?

@eikef
Copy link
Contributor

eikef left a comment

Will test these on Monday, probably won't have time for it today.

choices: ['present', 'absent']
default: 'present'
- On C(absent), the client will be removed if it exists.
type: str

This comment has been minimized.

@eikef

eikef Feb 22, 2019

Contributor

"str" is always the default and was intentionally omitted to keep in line with other modules in the repository; has this policy been changed so that the type has to be specified even if left at default, now? (same comment for all the 'type: str additions ;)

This comment has been minimized.

@dagwieers

dagwieers Feb 22, 2019

Author Member

Yes, we need to add for the documentation. Because nobody before documented the types (it wasn't even allowed) we don't know if an omission means a string or not.

BTW I proposed a PR to get this directly from the arg_spec but this one was rejected. Which means we have to document it explicitly.
BTW We also recommend to add type-information to the arg_spec for clarity, but required: no or default: None should not be added. Only type-information.

This comment has been minimized.

aliases:
- rootUrl
- Root URL appended to relative URLs for this client.
- This is C(rootUrl) in the Keycloak REST API.

This comment has been minimized.

@eikef

eikef Feb 22, 2019

Contributor

Is C() appropriate in this case? I had struggled to find the correct quoting for this as it does not specifically relate to ansible options or values thereof, but rather a reference to what the option is called in the REST API (which could potentially be different from what the module refers to, though right now every such option should have an equally-named alias).

This comment has been minimized.

@dagwieers

dagwieers Feb 22, 2019

Author Member

It makes it stand out as a function/variable, so yes I think it makes sense.

This comment has been minimized.

@dagwieers

dagwieers Feb 22, 2019

Author Member

right now every such option should have an equally-named alias

I have to say, I like that.

Show resolved Hide resolved lib/ansible/modules/identity/keycloak/keycloak_client.py Outdated

dagwieers added some commits Feb 17, 2019

keycloak_client: Fix validate-modules issues
This PR includes:

* Adding parameter types
* Fix validate-modules issue
* Improve parameter types and resulting changes
- Fix check-mode and return values

This PR needs to be verified and tested by maintainer(s).

@dagwieers dagwieers force-pushed the dagwieers:keycloak_client-validate branch from a4d722e to 56d516c Feb 22, 2019

@ansibot ansibot removed the needs_rebase label Feb 22, 2019

@ansibot

This comment was marked as outdated.

Copy link
Contributor

ansibot commented Feb 22, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/cloud/kubevirt/kubevirt_preset.py:0:0: E325 Argument 'resource_definition' in argument_spec defines type as <function list_dict_str at 0x7f7abc50d598> but documentation defines type as 'str'
lib/ansible/modules/cloud/kubevirt/kubevirt_rs.py:0:0: E325 Argument 'resource_definition' in argument_spec defines type as <function list_dict_str at 0x7f7abb2547b8> but documentation defines type as 'str'

click here for bot help

@ansibot ansibot added core_review and removed needs_revision labels Feb 22, 2019

@ansibot ansibot added the stale_ci label Mar 2, 2019

ndclt added a commit to ndclt/ansible that referenced this pull request Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.