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 an inherit_kerberos role #1574

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Sep 5, 2022

The role installs kerberos client packages
and optionally copies the configuration
and credential cache from the host machine.

The role installs kerberos client packages
and optionally copies the configuration
and credential cache from the host machine.
@wbclark
Copy link
Contributor Author

wbclark commented Sep 5, 2022

I've rewritten this to use the when: pathname is defined pattern

Comment on lines +2 to +14
- name: "Install client packages on Red Hat based distributions"
ansible.builtin.dnf:
name:
- "krb5-workstation"
- "krb5-libs"
state: present
when: ansible_os_family == "RedHat"

- name: "Install client packages on Debian based distributions"
ansible.builtin.apt:
name: "krb5-user"
state: present
when: ansible_os_family == "Debian"
Copy link
Member

Choose a reason for hiding this comment

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

you culd use ansible.builtin.package which works across distros, and load the correct list of packages using a vars file.

include_vars: "{{ ansible_os_family }}.yml"

bats_packages:

Not saying you have to, but something to consider :)

ansible.builtin.copy:
src: "{{ inherit_kerberos_ccache }}"
dest: "{{ inherit_kerberos_ccache }}"
owner: "{{ inherit_kerberos_local_user_name }}"
Copy link
Member

Choose a reason for hiding this comment

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

where does inherit_kerberos_local_user_name come from? I don't see it defined anywhere.

Copy link
Contributor Author

@wbclark wbclark Sep 6, 2022

Choose a reason for hiding this comment

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

This was originally written to be part of the unprivileged_user role and the value of unprivileged_user_username was passed for the owner. The approach was to give unprivileged_user additional features to have more general user creation/setup powers, while leaving its default behavior essentially the same in setting up the vagrant user for images that don't already have it; therefore the assumption at that time was that this block does not run by default, so an user passing the extra stuff required to run these extra bits would be doing it to setup their personal or devel user and not the default vagrant user in that role.

So with the request to split this out into more of a standalone role, instead I assume that the value will be provided by whatever role or playbook I create in the future that includes this role.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it!

@evgeni
Copy link
Member

evgeni commented Sep 6, 2022

Added some minor comments. Would be cool if there would be a few lines in the readme, how to use this role?

@wbclark
Copy link
Contributor Author

wbclark commented Sep 6, 2022

@evgeni some additional context --

The goal of these efforts is to create a portable hotfix environment which is fully encoded in a block of yaml so that other developers can simply copy & paste the box, edit as needed, and vagrant up

I could add a very basic README.md for the role if that is what we need to get it merged. The lack of documentation in this project is a real concern. If we do not also follow through on documenting the other roles then I am not sure whether it is worth documenting this one, and I don't have tons of time to devote to that larger effort at present.

What do you think?

@ekohl
Copy link
Member

ekohl commented Sep 6, 2022

I could add a very basic README.md for the role if that is what we need to get it merged. The lack of documentation in this project is a real concern. If we do not also follow through on documenting the other roles then I am not sure whether it is worth documenting this one, and I don't have tons of time to devote to that larger effort at present.

I think a policy where we document new roles is a good start.

On a related matter I opened #1568. It doesn't document the ansible roles, but that could be a nice addition.

@evgeni
Copy link
Member

evgeni commented Sep 6, 2022

I agree that the roles are under-documented, even if most roles are not designed to be used on their own, but are rather abstractions most users shouldn't have to care about.

If you say these are building blocks, and you will provide the "full" workflow later, I am cool with adding the docs for that feature later too.

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.

the existing comments can be adressed later

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