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 support for copying activation keys #157

Merged
merged 2 commits into from
Aug 6, 2018
Merged

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Aug 1, 2018

depends on SatelliteQE/nailgun#517 and still needs tests

Copy link
Member

@sean797 sean797 left a comment

Choose a reason for hiding this comment

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

This module needs some ansible_nailgun_cement love, you don't have to do it all if you don't want to. If you do, some tests would be awesome, again not required :-)

organization = self.find_organization(organization)

kwargs = {'name': name, 'organization': organization}
activation_key = self._entities.ActivationKey(self._server, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

I would rather find_activation_key was added to ansible_nailgun_cement.


if len(response) == 1 and len(new_response) == 0:
if not self._module.check_mode:
new_activation_key = response[0].copy(data={'new_name': new_name})
Copy link
Member

@sean797 sean797 Aug 2, 2018

Choose a reason for hiding this comment

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

I think adding an elif state == 'copied': to

def naildown_entity(entity_class, entity_dict, entity, state, module):
and using that could be useful for content_views as well.

organization=dict(required=True),
lifecycle_environment=dict(),
content_view=dict(),
subscriptions=dict(type='list'),
auto_attach=dict(type='bool', default=True),
state=dict(default='present', choices=['present', 'copied']),
),
supports_check_mode=True,
Copy link
Member

Choose a reason for hiding this comment

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

Adding required_if would be useful for new_name & state == copied

@evgeni
Copy link
Member Author

evgeni commented Aug 6, 2018

@sean797 I agree that this module needs some cement love, but I'd prefer not to mix it into this PR.

Copy link
Member

@sean797 sean797 left a comment

Choose a reason for hiding this comment

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

@evgeni okay - half doing the cement piece is probably not worth it.

One small comment but other than that LGTM 👍

@@ -210,13 +239,18 @@ def main():
password=dict(required=True, no_log=True),
verify_ssl=dict(type='bool', default=True),
name=dict(required=True),
new_name=dict(required=False),
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, we don't normally include required=False because it's the default.

@evgeni
Copy link
Member Author

evgeni commented Aug 6, 2018

@sean797 added some simple test, which will at the moment fail, as the code requires git master of nailgun

@sean797
Copy link
Member

sean797 commented Aug 6, 2018

@evgeni nice, now #159 is merged please rebase and 🍏

@evgeni
Copy link
Member Author

evgeni commented Aug 6, 2018

💚

@@ -1,3 +1,3 @@
localhost ansible_connection=local ansible_python_interpreter=../ansible_fail.py
#localhost ansible_connection=local ansible_python_interpreter=../ansible_fail.py
Copy link
Member Author

Choose a reason for hiding this comment

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

damn, that was not supposed to be here

Copy link
Contributor

@akofink akofink left a comment

Choose a reason for hiding this comment

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

Works beautifully. ACK!

@evgeni evgeni merged commit 00b290f into theforeman:master Aug 6, 2018
@evgeni evgeni deleted the ak-copy branch August 6, 2018 12:57
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.

3 participants