Good morning everyone. Does the preflight playbook have a bug? https://github.com/ceph/cephadm-ansible/blob/devel/cephadm-preflight.yml Line 82: paths: "{{ ['noarch', '$basearch'] if ceph_origin == 'community' else ['$basearch'] }}" The yum repo file then gets named ceph_stable_$basearch. Shouldn't it be basearch without the $ ? - Alex
I created a pull request, not sure what the etiquette is if I can merge it. First timer here.
On Fri, Apr 11, 2025 at 10:39 AM Alex <mr.alexey@gmail.com> wrote:
I created a pull request, not sure what the etiquette is if I can merge it. First timer here.
hi Alex, I cannot find your pull request in https://github.com/ceph/cephadm-ansible/ . did you create it in this project?
_______________________________________________ ceph-users mailing list -- ceph-users@ceph.io To unsubscribe send an email to ceph-users-leave@ceph.io
-- Regards Kefu Chai
On Thursday, April 10, 2025 1:08:00 AM Eastern Daylight Time Alex wrote:
Good morning everyone.
Does the preflight playbook have a bug?
https://github.com/ceph/cephadm-ansible/blob/devel/cephadm-preflight.yml
Line 82: paths: "{{ ['noarch', '$basearch'] if ceph_origin == 'community' else ['$basearch'] }}"
The yum repo file then gets named ceph_stable_$basearch.
Shouldn't it be basearch without the $ ?
Hi Alex, The `$basearch` there is likely trying to act as a yum (dnf) repo file variable. Take a look at https://developers.redhat.com/articles/2022/10/07/ whats-inside-rpm-repo-file and search in the page for `$basearch`. You will see it appear in various locations in the example file. This is a placeholder variable for dnf to replace with the actual base architecture of the system it is running on. See also https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/6/ html/deployment_guide/sec-using_yum_variables#sec-Using_Yum_Variables It's likely correct for the content of the repo file, but might be incorrect for the file name. I don't think simply removing the dollar sign would fix things. Perhaps a different suggestion would be to remove the variable from the file name parameter: ceph_stable_https://github.com/ceph/cephadm-ansible/blob/ 74520740e4f85ea001d7ca7ab6992ce66145bc3f/cephadm-preflight.yml#L99C24-L99C36 doing this should create mutiple repo defintions in a single repo file as per https://docs.ansible.com/ansible/latest/collections/ansible/builtin/ yum_repository_module.html (search for `Add multiple repositories into the same file`).
Thanks for the response John. We "spoke" on my PR for the log level set to DEBUG. I also have a PR open https://github.com/ceph/cephadm-ansible/pull/339 . I tested this one on my Ceph cluster. The issue which caused me to was that when I ran the preflight playbook it populated my /etc/yum.repos.d/ dir with a file called ceph_stable_$basearch (notice the dollar sign in the file name). The issue seems to be cosmetic since it still works, but still IMHO should be fixed. Upon looking into the file it also adds the $ into the name and description. The bug is really simple. It uses $basearch for the repo URL as it should, but it also uses the same variable with the dollar sign for the file name, repo name and description. My fix is to simply add a "| trim('$')" to remove the $ from the places where we don't want it. To me it seemed like the simplest solution, although may be not the most elegant. We can't simply add the dollar sign to baseurl: "{{ _ceph_repo.baseurl }}/{{ $item }}" since noarch is also a variable but it can't have a '$'. Since I was already fixing this, I fixed another cosmetic issue of the repo url having two forward slashes (//). Those come from the trailing slash in the variable as well as a slash in "{{ _ceph_repo.baseurl }}/{{ item }}" My first attempt was to remove the "/" from the line above but then I realized that if someone left it out from the variable it would break so I removed it from the variable but kept it in the code. If you agree that this is a bug and not intended to be that way then please take a look at my PR. I'm fairly comfortable with Ansible but first time Forking a repo so I'm sure I did it wrong so please let me know how to fix it and if I can actually make this VERY small contribution to the Ceph codebase that would be amazing. Thanks! Alex
On Wednesday, April 16, 2025 2:05:12 PM Eastern Daylight Time Alex wrote:
Hi. Any feedback on my pull request? Thanks, Alex.
Hi Alex, I'm not a maintainer of the ceph-ansible project. So I was not planning on reviewing it. That said the first change looks reasonable but I'm not sure about the merge commit and the commits after that. I would say it's up to the maintainers of cephadm-ansible to decide if they're ok with that. In projects I do help maintain I like to see "clean" PRs without merges and fixup commits in them. Good luck!
Thanks John. Doing some quick Googling it seems that I can clean up an existing PR without making a new one? - Alex
On Thu, Apr 17, 2025 at 5:44 AM Alex <mr.alexey@gmail.com> wrote:
Thanks John.
Doing some quick Googling it seems that I can clean up an existing PR without making a new one?
Yes, you can amend your pull request by force pushing it. see also https://docs.ceph.com/en/latest/dev/developer_guide/basic-workflow/ .
- Alex _______________________________________________ ceph-users mailing list -- ceph-users@ceph.io To unsubscribe send an email to ceph-users-leave@ceph.io
-- Regards Kefu Chai
participants (3)
-
Alex
-
John Mulligan
-
kefu chai