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

Use ansible_user_dir in customize_home role #1760

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Feb 8, 2024

This was only working when run under the vagrant user

@wbclark wbclark changed the title Use ansible_env.HOME in customize_home role Use ansible_user in customize_home role Feb 9, 2024
@@ -1,11 +1,11 @@
- name: Check that the .env exists
stat:
path: /home/vagrant/foreman/.env
path: "/home/{{ ansible_user }}/foreman/.env"
Copy link
Member

Choose a reason for hiding this comment

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

How about

Suggested change
path: "/home/{{ ansible_user }}/foreman/.env"
path: "{{ ansible_user_dir }}/foreman/.env"

(from https://github.com/ansible/ansible/blob/stable-2.16/lib/ansible/module_utils/facts/system/user.py)

This should be present in any run that has facts enabled, but has the benefit of not guessing the home dir as /home/$USERNAME but actually looking it up from the user database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll give it a test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only works if I manually add ansible_user_dir to the variables for the ansible provisioner in the box yaml config... otherwise (I guess because become: true in playbooks/katello_devel.yml) it still tries to use /root

I also realized that I was already overriding the value for ansible_user in my box, so that is likely the only reason my other approach was working (and I must have encountered something like this before already)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose I could just add a role variable here with a default value that works for most setups to avoid this annoying behavior with become

Copy link
Member

Choose a reason for hiding this comment

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

ah, yeah… become certainly kills that.

role variable sounds good to me then!

Copy link
Member

Choose a reason for hiding this comment

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

Another idea, can we force become: false when running this role?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another idea, can we force become: false when running this role?

See the thread above this one that I marked as "resolved" :(

but with become: false it fails due to permission denied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another idea, can we force become: false when running this role?

although I was using become: false for just the one task and not the whole role, and the user should have permission to modify the contents of their own home directory....

Copy link
Contributor Author

@wbclark wbclark Feb 22, 2024

Choose a reason for hiding this comment

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

OK, the issue appears to be that we have become: true in playbooks/katello_devel.yml and specifying the become behavior at the more granular level of roles or tasks was not changing the value of variables like ansible_user_dir that get determined statically.

The good news is that it looks like become works for role dependencies in meta/main.yml, so I can remove it from playbooks/katello_devel.yml and add it back as needed per role dependency in roles/katello_devel/meta/main.yml

ekohl
ekohl previously requested changes Feb 16, 2024
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Actually, the commit message no longer matches the content of the commit.

@wbclark
Copy link
Contributor Author

wbclark commented Feb 22, 2024

Actually, the commit message no longer matches the content of the commit.

I haven't gotten back implementing the role variable yet either. I was out sick, but will get to that today

Forcing become: true on the entire role was breaking the behavior
of the customize_home role dependency
This was only working when run under the vagrant user
@wbclark wbclark changed the title Use ansible_user in customize_home role Use ansible_user_dir in customize_home role Feb 23, 2024
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.

We should drop the scl roles, they would need become true also, but are unused.

roles/katello_devel/meta/main.yml Show resolved Hide resolved
These were no longer used as we don't need EL7 devel envs.
Of these roles, the postgresql_scl and nodejs_scl roles
are not used elsewhere in the project, and are dropped entirely.
The ruby_scl role remains as it is still used in hammer and
dynflow devel envs.
@wbclark
Copy link
Contributor Author

wbclark commented Feb 26, 2024

We should drop the scl roles, they would need become true also, but are unused.

I pushed a change that removes the SCL roles from katello_devel

Two of these were now also unused in other roles and playbooks, so I removed those roles entirely.

@wbclark
Copy link
Contributor Author

wbclark commented Feb 26, 2024

We should drop the scl roles, they would need become true also, but are unused.

Eh, so I checked /roles and /playbooks but these were in fact still used in the katello_devel pipeline.

I've updated that to use the centos8-stream box as a base now, and removed the SCL roles there as well.

foreman_installer_scenario: katello-devel
foreman_installer_additional_packages:
- foreman-installer-katello
foreman_installer_disable_system_checks: true
foreman_installer_options_internal_use_only:
- "{{ '--katello-devel-scl-ruby=' + ruby_scl_version if ansible_distribution_major_version == '7' else '' }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(this was already removed in puppet-katello_devel)

@wbclark
Copy link
Contributor Author

wbclark commented Feb 27, 2024

Also, if you prefer the removal of unused SCL roles and change to centos8-stream in katello-katello devel pipelines in separate PRs, let me know and I'll make the change

@wbclark wbclark requested review from ekohl and evgeni February 28, 2024 21:52
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.

There are separate commits, so this looks good to me!

@wbclark
Copy link
Contributor Author

wbclark commented Feb 29, 2024

@ekohl would you be willing to remove the out of date "changes requested" flag please?

@wbclark
Copy link
Contributor Author

wbclark commented Feb 29, 2024

What sorcery is that! Didn't realize it was possible.

Also, I cannot merge in this repository -- would you please, @evgeni ?

@evgeni evgeni merged commit f42a9b6 into theforeman:master Feb 29, 2024
8 checks passed
@evgeni
Copy link
Member

evgeni commented Feb 29, 2024

I think this sorcery is limited to people with commit rights ;-)

(done)

@wbclark
Copy link
Contributor Author

wbclark commented Feb 29, 2024

TYVM!

@wbclark wbclark deleted the ansible_user_home branch February 29, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants