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

Create foreman_user module #296

Merged
merged 7 commits into from
Jun 18, 2019
Merged

Create foreman_user module #296

merged 7 commits into from
Jun 18, 2019

Conversation

ephracis
Copy link
Contributor

@ephracis ephracis commented May 30, 2019

Create a new module for managing users. In order to change password we
need to force the password field to be sent during POST but that
field is not included when doing GET, so we put it in check_missing.

I noticed, though, that the value for check_missing was a variable
from a separate loop which seems wrong. Instead I change it to get the
value from the incoming desired dict. This makes the module behave as
we want.

To ensure that the new user works we do an API request as the
newly created user. We do this again after changing password to ensure it worked.

Closes #200

}

# List of allowed timezones
timezone_list = [
Copy link
Member

Choose a reason for hiding this comment

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

Can we pull these from apypie? It's should be exposed via https://github.com/Apipie/apypie/blob/master/apypie/param.py#L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be awesome. I just need to figure out how to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First of all, I don't understand Apypie well enough to know how to fetch that data from the server.

Secondly, as far as I can tell when checking /apidoc/v2.json is that the validator field just contains a human description and I'm not sure if that is stable enough for us to parse it and use it for validation of the module params.

Perhaps we could replace the hardcoded list in a later PR?

Copy link
Member

Choose a reason for hiding this comment

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

I was just about to comment something similar. We're going to need changes to apypie. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Can you please open an issue against apypie? The validators can use some love indeed ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -200,7 +200,7 @@ def update_resource(self, resource, old_entity, entity_dict, check_missing, chec
if check_missing is not None:
for key in check_missing:
if key in entity_dict and key not in volatile_entity.keys():
volatile_entity[key] = new_value
volatile_entity[key] = entity_dict[key]
Copy link
Member

Choose a reason for hiding this comment

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

oups. :)
Good catch.

@mdellweg
Copy link
Member

mdellweg commented Jun 5, 2019

This probably also needs a rebase and the XDG_CACHE_HOME thing

@ephracis
Copy link
Contributor Author

ephracis commented Jun 6, 2019

This probably also needs a rebase and the XDG_CACHE_HOME thing

Done!

RETURN = ''' # '''

from ansible.module_utils.foreman_helper import (
sanitize_entity_dict,
Copy link
Member

Choose a reason for hiding this comment

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

you know ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I know... ;)

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

One comment/question. Looks good from a beach 5000 miles away ;)

@evgeni
Copy link
Member

evgeni commented Jun 6, 2019

Argh, seems my client didn't post the actual question.

Does the if mail not in entity_dict part work also when entity is None as when the user is freshly created? Or is mail mandatory for new creations?

@evgeni
Copy link
Member

evgeni commented Jun 6, 2019

(it is from the description, wonder if we can make that more explicit in the code)

@ephracis
Copy link
Contributor Author

ephracis commented Jun 6, 2019

(it is from the description, wonder if we can make that more explicit in the code)

Yeah I was thinking that myself. But it's kinda hard to do since we need to first check if the user exists before we know whether some params are required or not.

So I think the best and most boring solution would be to just document the requirement and let the error from Foreman bleed through the module. Maybe in the future we could improve it further.

@ogajduse ogajduse mentioned this pull request Jun 6, 2019
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Overall, nothing scary. And i like the twist, where you use the newly created user in the tests.

}

# List of allowed timezones
timezone_list = [
Copy link
Member

Choose a reason for hiding this comment

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

Are these lists (until they are generated) better suited to live somewhere in module_utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we would use them in multiple modules they would be. But I'm not sure we will do that anytime soon. Do we want to stuff a lot of into the utils that is only used in a single module?

I vote for keeping stuff in modules, and then move into module_utils when it needs to be shared between two or more modules. What do you guys think?

gather_facts: false
tasks:
- name: Load server config
include_vars:
Copy link
Member

Choose a reason for hiding this comment

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

I guess, you need to adjust that now.
(Directory changed...)

Create a new module for managing users. In order to change password we
need to force the `password` field to be sent during POST but that
field is not included when doing GET, so we put it in `check_missing`.

I noticed, though, that the value for `check_missing` was a variable
from a separate loop which seems wrong. Instead I change it to get the
value from the incoming desired dict. This makes the module behave as
we want.

To ensure that the new user works we do a minor API request as the
newly created user.
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Yes that should be it. Pending travis...

@mdellweg mdellweg merged commit 9ccd95c into theforeman:master Jun 18, 2019
@mdellweg
Copy link
Member

@ephracis Thank you

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.

Create foreman_user module
4 participants