Skip to content

Move ansible_user_dir to group_vars #3471

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 24, 2025
Merged

Move ansible_user_dir to group_vars #3471

merged 1 commit into from
Nov 24, 2025

Conversation

@amartyasinha
Copy link
Contributor

@amartyasinha amartyasinha commented Nov 10, 2025 •
edited
Loading

This commit is one of the steps of replacing common used vars with group_vars to improve overall maintenance of variables in ci-framework

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 10, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 10, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@amartyasinha
Copy link
Contributor Author

amartyasinha commented Nov 11, 2025

recheck

@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@amartyasinha
Copy link
Contributor Author

amartyasinha commented Nov 12, 2025

recheck

@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@michburk
Copy link
Contributor

michburk commented Nov 12, 2025

recheck

@softwarefactory-project-zuul

This comment was marked as outdated.

Sign in to view
@amartyasinha
[multiple] Move ansible_user_dir to group_vars
d0a4d08 
This commit is one of the steps of replacing common used vars with group_vars to improve overall maintenance of variables in ci-framework

Signed-off-by: Amartya Sinha <[email protected]>
@amartyasinha
Copy link
Contributor Author

amartyasinha commented Nov 19, 2025

Ammend commit message to make GH jobs pass. No file changes was made in this push.

@amartyasinha amartyasinha marked this pull request as ready for review November 20, 2025 13:18
@amartyasinha amartyasinha changed the title DNM: Move ansible_user_dir to group_vars Move ansible_user_dir to group_vars Nov 20, 2025
abays
abays reviewed Nov 20, 2025
View reviewed changes
Copy link
Contributor

@abays abays left a comment

Choose a reason for hiding this comment

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

Succeeded locally with VA1

@hjensas
Copy link
Contributor

hjensas commented Nov 21, 2025

I don't have a strong opinion. This is probably fine, and the following is probably my lack of ansible understanding. :)

What I do wonder is ansible_user_dir, prior to this change it was set via lookup of HOME from env.
As I understand, it would then be whatever the homedir for the user ansible uses to connect to that specific host?
After this change, are we setting it in group_vars? If that is the case, will the user and homedir be the same for all hosts in a group in the inventory?

Also, ansible_user_dir is a fact? https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/facts/system/user.py#L46 can we not just use that, why group_vars ?

@amartyasinha
Copy link
Contributor Author

amartyasinha commented Nov 21, 2025

What I do wonder is ansible_user_dir, prior to this change it was set via lookup of HOME from env. As I understand, it would then be whatever the homedir for the user ansible uses to connect to that specific host? After this change, are we setting it in group_vars? If that is the case, will the user and homedir be the same for all hosts in a group in the inventory?

Yes, we have setup ansible_user_dir in group_vars.

Also, ansible_user_dir is a fact? https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/facts/system/user.py#L46 can we not just use that, why group_vars ?

@hjensas ++ Thanks for pointing it out. Since we were setting this var via lookup of HOME from env, I thought it is our own defined variable, and consolidated the definition at single place (group_vars). Maybe in next iteration, I can try using value of ansible_user_dir from set facts instead of group_vars?

abays
abays approved these changes Nov 21, 2025
View reviewed changes
Copy link
Contributor

@abays abays left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 21, 2025
@hjensas
Copy link
Contributor

hjensas commented Nov 21, 2025

/lgtm

Thanks @amartyasinha - I have no objections to merging this PR.

@hjensas
Copy link
Contributor

hjensas commented Nov 21, 2025

/lgtm

@danpawlik danpawlik merged commit b312943 into openstack-k8s-operators:main Nov 24, 2025
14 of 15 checks passed
hjensas added a commit to hjensas/ci-framework that referenced this pull request Nov 24, 2025
@hjensas
[ci_nmstate] Check if kubeconfig file exists
ec1763b 
The ci_nmstate role was failing during adoption deploy-infra jobs when
cifmw_openshift_kubeconfig was defined but the file didn't exist yet.

Root cause: ci-framework-jobs' adoption-uni-job-base uses variable_files_dirs
to scan all YAML files in the scenario directory, including 05-tests.yaml
which sets cifmw_openshift_kubeconfig. This has been the case since adoption
jobs were introduced in October 2024.

However, during the deploy-infra phase (before deploy-ocp), the OCP cluster
and kubeconfig file don't exist yet. The issue was likely exposed by PR openstack-k8s-operators#3471
which changed how ansible_user_dir is evaluated, affecting how/when the
kubeconfig path gets resolved.

Fix: Add "cifmw_openshift_kubeconfig is exists" check to tasks that use the
kubeconfig. The existing code already handles the skipped task gracefully
via default([]) safeguards, treating all hosts as "unmanaged" when no k8s
cluster is available (which is correct for infra creation).

Fixes: OSPCIX-1122
Related: openstack-k8s-operators#3471
Assisted-By: Claude Code/claude-4.5-sonnet
Signed-off-by: Harald Jensås <[email protected]>
@hjensas hjensas mentioned this pull request Nov 24, 2025
Closed
This was referenced Dec 2, 2025
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abays abays abays approved these changes

@danpawlik danpawlik danpawlik approved these changes

@hjensas hjensas hjensas approved these changes

@tosky tosky tosky approved these changes

@imatza-rh imatza-rh Awaiting requested review from imatza-rh

@eurijon eurijon Awaiting requested review from eurijon

@eshulman2 eshulman2 Awaiting requested review from eshulman2

Assignees

@abays abays

@hjensas hjensas

Labels

lgtm Ready For Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@amartyasinha @michburk @hjensas @abays @danpawlik @tosky