-
Notifications
You must be signed in to change notification settings - Fork 144
[multiple] Improve cifmw_project_dir group var name #3583
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
base: main
Are you sure you want to change the base?
[multiple] Improve cifmw_project_dir group var name #3583
Conversation
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
95f3e9c to
e5f9b2d
Compare
|
zuul gate jobs were stuck for unknown reason. this force commit adds nothing, just meant to retrigger the jobs. |
|
recheck |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
e5f9b2d to
e25bf75
Compare
|
Merging blocked as gate job |
|
recheck |
| cifmw_project_dir: src/github.com/openstack-k8s-operators/ci-framework | ||
| cifmw_project_dir_absolute: "{{ ansible_user_dir }}/{{ cifmw_project_dir }}" | ||
| ci_framework_repo_relative: src/github.com/openstack-k8s-operators/ci-framework | ||
| ci_framework_repo: "{{ ansible_user_dir }}/{{ ci_framework_repo_relative }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: These names break from the pattern of prefixing these vars with cifmw_, maybe something like cifmw_framework_repo would be better? Open to disagreement or other suggestions, though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cifmw stands for ci-framework. So, cifmw_framework_repo would mean ci-framework_framework_repo which will not make sense.
Forcing cifmw prefix does not make sense to me. According to Ansible best practices, vars within a role should have role prefix, but there is no such requirement or convention for group_vars.
The reason these vars does not start with cifmw is to maintain the consistency with D/S vars definition. We have vars name like ci_framework_repo in D/S. Please feel free to recommend some other better var name if you can think of any.
Changing the variable to ensure all variable names are following the similar pattern. - Now, cifmw_project_dir will be ci_framework_repo_relative and cifmw_project_dir_absolute will be ci_framework_repo - Since the absolute values are used in the entire framework, that variable should not have extra keyword (absolute). - The reason to move from `dir` to `repo` is to match the pattern of other variables. Signed-off-by: Amartya Sinha <[email protected]>
e25bf75 to
c1ea450
Compare
Changing the variable to ensure all variable names are following the similar pattern.
cifmw_project_dirwill beci_framework_repo_relativeandcifmw_project_dir_absolutewill beci_framework_repo.dirtorepois to match the pattern of other variables.Depends-On: openstack-k8s-operators/tcib#356