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 the Kubernetes Ansible collection #2646

Merged
merged 5 commits into from
Mar 13, 2020

Conversation

fabianvf
Copy link
Member

Description of the change:
Updates scaffolded code and tests to use the Kubernetes modules from the Kubernetes Ansible collection instead of Ansible core. Existing code should work for the time being, so no immediate steps are required for existing users, though it is recommended they switch to using the collection to continue getting new fixes and features.

Motivation for the change:
Active module development has moved to the Kubernetes Ansible collection. At some point in the future, support for the Kubernetes modules in Ansible core may be removed.

@fabianvf fabianvf requested a review from geerlingguy March 12, 2020 18:03
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 12, 2020
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Great @fabianvf.

IHMO just fix the CI and then it is
/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2020
@@ -155,7 +155,7 @@ would become:

```yaml
- name: Create bind credential secret
k8s:
community.kubernetes.k8s:
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that there's a different way (which I prefer) to upgrade to using collections, and that is to add a collections keyword on the play level, listing the collections you use, then leave the module invocations the same. Otherwise you end up writing things like community.kubernetes.k8s over and over and over, getting carpal tunnel.

Example:

- hosts: hosts_here
  collections:
    - community.kubernetes
    - operator_sdk.util
  tasks:
    - k8s:
      ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I put in the FQCN here and in other examples because they're outside of the context of a playbook and it didn't feel like an appropriate spot to explain collections and tell users to add those to their playbooks, though I did update the scaffolded code to use the collection play keyword when generating the playbook. Since it's possible to generate just a role, no playbook, and AFAIK there is no way to pass the collection keyword when running a role directly, I do think it makes sense to include the FQCN outside of playbook examples. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK there is no way to pass the collection keyword when running a role directly

I haaaaaaaaate that. I'm hoping to campaign for that not to be the case because I absolutely hate the idea of having to type out a FQCN every time you type in a module in Ansible.

@@ -31,6 +31,11 @@ Python][openshift_restclient_python] package. This can be installed from pip:
$ pip3 install openshift
```

Finally, a user must install the Ansible Kubernetes collection from ansible-galaxy:
```bash
$ ansible-galaxy install community.kubernetes
Copy link
Contributor

Choose a reason for hiding this comment

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

This would need to be ansible-galaxy collection install community.kubernetes (at least for now). Otherwise we can use a requirements.yml file and just have it be ansible-galaxy install -r requirements.yml (and that could have any possible versioning requirements and other collections added as needed...

Copy link
Contributor

Choose a reason for hiding this comment

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

(I think I would prefer a requirements.yml file, even if it didn't have a version constraint, so it would be like:

---
collections:
  - name: community.kubernetes

Copy link
Member Author

Choose a reason for hiding this comment

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

I always mess that up, good catch. As for the requirements.yml file, I'd also like to do it that way, but I think it's probably out of scope for this PR and should be done in a followup if we want to get it in

@@ -61,7 +66,7 @@ we will create and delete a namespace with the switch of a variable:
```yaml
---
- name: set example-memcached namespace to {{ state }}
k8s:
community.kubernetes.k8s:
Copy link
Contributor

Choose a reason for hiding this comment

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

(Same notes as above on the collections keyword... I just hate seeing FQCNs everywhere, Ansible starts looking like Java...)

@@ -79,7 +79,7 @@ RUN yum clean all && rm -rf /var/cache/yum/* \
&& yum remove -y gcc libffi-devel openssl-devel python36-devel \
&& yum clean all \
&& rm -rf /var/cache/yum \
&& ansible-galaxy collection install operator_sdk.util
&& ansible-galaxy collection install operator_sdk.util community.kubernetes
Copy link
Contributor

Choose a reason for hiding this comment

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

See note about requirements.yml file from earlier.

@geerlingguy
Copy link
Contributor

Related upstream issue, to make maintaining version compatibility easier with upstream Ansible collections: ansible/ansible#68194

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2020
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

Comment on lines +14 to +15
RUN ansible-galaxy collection build /tmp/fixture_collection/ --output-path /tmp/fixture_collection/ \
&& ansible-galaxy collection install /tmp/fixture_collection/operator_sdk-test_fixtures-0.0.0.tar.gz
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, prevents a new layer by combining.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2020
@fabianvf fabianvf merged commit 373ebf3 into operator-framework:master Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants