-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Adds support for Ansible collections #9582
Conversation
Hi @luksi1. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
b54144c
to
ccf8c8c
Compare
021ba50
to
1a7f708
Compare
f6ff021
to
ede4218
Compare
9bdc69b
to
bdf9ad3
Compare
aedd841
to
2bcf169
Compare
Sorry, for the late reply @floryut. Yes, there's some problems that popped up with syntax checking that I need to resolve. |
7dfef09
to
613f26f
Compare
@floryut, I think this is good-to-go. Sorry, for the delay. I made a last minute change in |
@luksi1 Thank you 👍 |
@@ -0,0 +1,357 @@ | |||
#!/usr/bin/python |
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.
I don't see this as removed elsewhere are we supposed to have two versions of this now?
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.
Unfortunately, the answer is yes. We would have to maintain a duplicate of this since an Ansible collection structure places libraries under plugins
, see here, while an Ansible playbook structure
places libraries under library
, see here.
The idea of this first iteration is to retain the structure as-is, so users could either use the repository as an Ansible collection, by simply pointing their requirements.txt directly at the Kubespray's repository, or download it and use it the same as before. And this seemed like the easiest way forward.
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.
Random guess but did you try to see if a symlink works here?
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.
It would probably work, but before I start changing anything, do we want to go down this path? Symlinks will probably work fine on Linux distributions, and should work out-of-the-box when cloning (not tested), but I believe that symlinks in Windows require admin rights to access mklink functions plus enabling core.symlinks
functionality when cloning, which might result in some confusion for users running Ansible from Windows. There might even be some additional edge cases that I'm not familiar with. Are we comfortable with this?
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.
Windows without WSL is not natively supported as a control node.
https://docs.ansible.com/ansible/latest/installation_guide/intro_installation.html#control-node-requirements
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.
That settles it :) I'll see if I can't get the collection working with a symlink instead of duplicating the library. Thanks for the feedback.
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.
Btw. how about using #!/usr/bin/env bash
everywhere or at least in all new places introduced with this PR? This way operating systems like NixOS are supported, too. Using env
is the best way when going for maximum portability. https://www.gnu.org/software/coreutils/manual/html_node/env-invocation.html
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.
Since CI is failing I am guessing you might try the other way around (moving the file to the new location and symlink from old to new).
@michel-zimmer AFAICS there is only one file in this PR with this https://github.com/kubernetes-sigs/kubespray/blob/46dc09ca7647213230a0d869b13e150ae650c2b4/tests/scripts/check_galaxy_version.sh, feel free to send a PR for cleaning up those across the repo 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.
@MrFreezeex, good suggestion! Worked like a charm! Let me know if there's any other concerns regarding this PR.
Thanks for modernizing Kubespray with collection support :D @luksi1 |
What type of PR is this?
What this PR does / why we need it:
Collections are a modern way for Ansible to handle projects like this, allowing downstream users to patch into kubespray's code. This change allows for example a downstream user to simply install a Kubernetes cluster with something like this:
Which issue(s) this PR fixes:
Fixes #9048
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
This would cause a change to the repository's structure, meaning downstream users would either need to change their code to point to the
playbooks
directory or use theansible.builtin.import_playbook
module