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

krb_ticket: Create module #8953

Merged
merged 31 commits into from
Oct 10, 2024
Merged

Conversation

abakanovskii
Copy link
Contributor

@abakanovskii abakanovskii commented Oct 1, 2024

SUMMARY

I created a krb_ticket module to serve as wrapper around kinit/klist/kdestroy

ISSUE TYPE
  • New Module/Plugin Pull Request
COMPONENT NAME

krb_ticket

ADDITIONAL INFORMATION

So It is a simple yet very useful wrapper around krb5 base utilities because although It is used quite often there are not module to manage these without using shell/command modules
I took @russoz suggestion about simple unit testing using Helper class with yaml, thanks you very much :)

# Before this module
- name: Variant 1
  ansible.builtin.shell: kinit admin
  args:
    stdin: "{{ admin_password }}"
  changed_when: true

- name: Variant 2
  ansible.builtin.shell: echo {{ admin_password }} | kinit admin
  changed_when: true
  
# Now you can do something like this
- name: Will return CHANGED
  community.general.krb_ticket:
    principal: admin
    password: "{{ admin_password }}"

- name: Will return OK  
  community.general.krb_ticket:
    principal: admin
    password: "{{ admin_password }}"

@ansibullbot ansibullbot added module module new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 1, 2024
@abakanovskii
Copy link
Contributor Author

abakanovskii commented Oct 1, 2024

Hello @russoz! Could you please provide some help for using Helper class to unit test
It says that it can not recognize mock path although when using directly like this it should work as intended

        self.mock_run_command = patch('%s.run_command' % ansible_module_path)
        self.module_main_command = self.mock_run_command.start()
        self.mock_get_bin_path = patch('%s.get_bin_path' % ansible_module_path)
        self.get_bin_path = self.mock_get_bin_path.start()
        self.get_bin_path.return_value = '/testbin/kinit'

image

@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 Oct 1, 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.

The YAML spec used has changed - apologies, I should have let you know - and the outcome of the old test structure running in the new code is that get_bin_path() does not get to be mocked, therefore it fails to find the binary and the problem cascades.

Thanks for exposing this, please do apply the necessary changes, whilst I update the docs and the code to be more comprehensive and resilient.

plugins/modules/kutils.py Outdated Show resolved Hide resolved
tests/unit/plugins/modules/test_kutils.yaml Outdated Show resolved Hide resolved
tests/unit/plugins/modules/test_kutils.py Outdated Show resolved Hide resolved
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.

Couple of comments about your code.

plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
@ansibullbot

This comment was marked as outdated.

@ansibullbot

This comment was marked as outdated.

@russoz
Copy link
Collaborator

russoz commented Oct 2, 2024

If I may suggest, give it a try to andebox by yours truly:

https://pypi.org/project/andebox/

You can run this sanity test easily in your local machine by simply running:

$ andebox test -- sanity --docker default --python 3.10 plugins/modules/kutils.py

@abakanovskii
Copy link
Contributor Author

If I may suggest, give it a try to andebox by yours truly:

https://pypi.org/project/andebox/

You can run this sanity test easily in your local machine by simply running:

$ andebox test -- sanity --docker default --python 3.10 plugins/modules/kutils.py

Thats very useful, thanks!

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 2, 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! I have a few docs comments :)

plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
plugins/modules/kutils.py Outdated Show resolved Hide resolved
@abakanovskii
Copy link
Contributor Author

Thanks for the review! @felixfontein

plugins/modules/kutils.py Outdated Show resolved Hide resolved
renewable:
description:
- Requests renewable tickets, with a total lifetime equal to O(renewable).
- "The value for O(renewable) must be followed by one of the following delimiters: V(s) - seconds, V(m) - minutes, V(h) - hours, V(d) - days."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some wordsmithing: delimiters usually mean something between things - not the case here. Maybe:

Suggested change
- "The value for O(renewable) must be followed by one of the following delimiters: V(s) - seconds, V(m) - minutes, V(h) - hours, V(d) - days."
- "The value for O(renewable) must be followed by one of the following suffixes: V(s) - seconds, V(m) - minutes, V(h) - hours, V(d) - days."

?

Comment on lines 319 to 321
required_by={
'keytab_path': 'keytab'
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs for keytab_path state that when it is provided, then keytab must be True. This required_by will not guarantee you that - it will only guarantee that keytab is not None. Maybe the module needs to check and raise and exception if keytab_path is not None and keytab is not True.

plugins/modules/kutils.py Outdated Show resolved Hide resolved
@felixfontein felixfontein removed the backport-9 Automatically create a backport for the stable-9 branch label Oct 7, 2024
@abakanovskii abakanovskii changed the title kutils: Create module krb_ticket: Create module Oct 8, 2024
@abakanovskii
Copy link
Contributor Author

I changed the name from kutils to krb_ticket to make it more clear for future users

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.

LGTM

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Oct 10, 2024
@felixfontein felixfontein merged commit 3de4682 into ansible-collections:main Oct 10, 2024
126 checks passed
@felixfontein
Copy link
Collaborator

@abakanovskii thanks for your contribution!
@russoz thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module module new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants