Bugfix/code inspection

[bbaassssiiee]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

• Using ansible-core we need to know the ansible-collections used.
• Updated ansible in requirements.txt because it was yanked.
• Using ansible-lint it can test by downloading the collections
• We can simply run ansible-lint in the repo with ansible-core installed.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
I tested the Ansible in this repo with ansible-core+ansible-lint, the collections were not declared explicitly so ansible-lint was unsuccessfull.

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @bbaassssiiee. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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-sigs/prow repository.

[MrFreezeex]

Hi thanks for your contribution!

I am bit confused by the yamllint updates as we are running yamllint 1.35.1, the yaml lint changes that you made looks relatively sane except for comments-indentation: false and the octal parameters that you added seems to be the default as well, so unsure we should set those explicitly.

For the update on collections could you share what part of kubespray requires those collections? AFAIK most of these collections are only required on playbooks in contrib/

[bbaassssiiee]

Thanks for the feedback. I see you depend on the ansible python module. A more modern approach would be to depend on ansible-core and to declare the collections used, as I did.

[MrFreezeex]

Thanks for the feedback. I see you depend on the ansible python module. A more modern approach would be to depend on ansible-core and to declare the collections used, as I did.

Well sure that's doable, however I'm pretty sure the collections you added doesn't map to things we actually use in kubespray. Maybe for ansible.netcommon but in any case please provide for each of those at least one place where we use the corresponding collection.

[bbaassssiiee]

OK, I updated yamllint to 1.35.1, and ran ansible-lint 24.7.0 from the root directory of the repo.

---
WARNING  Found incompatible custom yamllint configuration (.yamllint), please either remove the file or edit it to comply with:
  - comments.min-spaces-from-content must be 1
  - comments-indentation must be false
  - octal-values.forbid-implicit-octal must be true
  - octal-values.forbid-explicit-octal must be true.
 ...

Read https://ansible.readthedocs.io/projects/lint/rules/yaml/ for more details regarding why we have these requirements. Fix mode will not be available.

[bbaassssiiee]

In the same ansible-lint run this error appeared that can be resolved by installing the collection

contrib/network-storage/glusterfs/roles/glusterfs/server/tasks/main.yml:57:3: syntax-check[unknown-module]: couldn't resolve module/action 'gluster.gluster.gluster_volume'. This often indicates a misspelling, missing collection, or incorrect module path.

[bbaassssiiee]

The blessed way to write octal values nowadays is:

- ansible.builtin.file:
    path: /etc/host
    state: file
    mode: "0644"
    owner: root
    group: root

The additions for .yamllint will surface these errors:

333 yaml[octal-values]           basic   formatting, yaml

[bbaassssiiee]

The remainder is then:

15 jinja[spacing]               basic   formatting (warning)

[bbaassssiiee]

With regards to the use of the kubernetes.core collection:

$ find tests -name "*.yml" | xargs grep -n kubernetes.core .
grep: .: Is a directory
tests/cloud_playbooks/roles/packet-ci/tasks/cleanup-old-vms.yml:4:  kubernetes.core.k8s_info:
tests/cloud_playbooks/roles/packet-ci/tasks/create-vms.yml:25:  kubernetes.core.k8s:
tests/cloud_playbooks/roles/cleanup-packet-ci/tasks/main.yml:4:  kubernetes.core.k8s_info:

[bbaassssiiee]

Note: Running pre-commit on AlmaLinux8 requires ruby-devel just for markdownlint

[bbaassssiiee]

I amended my commit with references, please review again. I'll assume you agree with stricter code-checking.

[MrFreezeex]

I'm not entirely sure of adding this here as we don't have a collections dir so far. Maybe it would be better to only document this here: https://github.com/kubernetes-sigs/kubespray/blob/master/docs/ansible/ansible_collection.md WDYT?

[bbaassssiiee]

ansible-lint will look at this file when you simply run it in the top of the repo with only ansible-core installed. After commit cf56057 the error syntax-check[unknown-module] was resolved.

[MrFreezeex]

It also looks at the root dir it seems (https://github.com/ansible/ansible-lint/blob/288db8a90d640cea6a1b182ca0db22c89d06b49c/docs/usage.md?plain=1#L322) so would be preferable to be there IMO

[MrFreezeex]

Actually we have some dependencies directly in galaxy.yml so the missing ones should just be added there

[bbaassssiiee]

I moved the requirements.yml to the root, and moved gluster.gluster in tests/requirements.yml.
This way we can simply run ansible-lint in the root with ansible-core, and that will pass now.

[bbaassssiiee]

The galaxy file is for the collection.

[MrFreezeex]

I would prefer to use the galaxy.yml directly. I am not sure we should support the combination of using ansible-core and not using the kubespray collection as well. This improvement is about modernizing kubespray in some sort and using kubespray via the collection is technically the modern way of using kubespray.

Right now we mandate users to install kubespray requirements via requirements.txt so what you are trying to do does not go into this direction. I would be ok having this expressed via the kubespray collection/Galaxy file as there are already collection requirements there although they are not complete. Also galaxy.yml is parsed by ansible-lint so even that use case is getting covered.

[bbaassssiiee]

Ok, I'll add them to galaxy.yml.
BTW. requirements.txt lists a yanked version of Ansible. I'll add a commit.

[MrFreezeex]

Could you split this into two PR? Now that you fixed the octal quotes this is becoming bigger and it would be easier to separate the collection dependencies and the yamllint fixes. Thanks!

[bbaassssiiee]

Sorry, Found one more thing. The pre-commit hook installed different versions of python modules than stated in requirements.txt.

Rather create very small PRs?

[MrFreezeex]

No worries, thanks for the catch. Sounds good to keep that in this PR 👍
/lgtm

[yankay]

Thanks @bbaassssiiee

Could we squash the commits, and add the `question Does this PR introduce a user-facing change?: ' to the PR description?

[bbaassssiiee]

I squashed my commits and signed-off on this PR. Great working with you, thanks!

[mzaian]

I squashed my commits and signed-off on this PR. Great working with you, thanks!

Thank you and also add a release note.

[bbaassssiiee]

Please elaborate. It's unclear to me what more I need to do to get this merged. This PR does not introduce a user-facing change.

[yankay]

Thanks @bbaassssiiee
/approve

[bbaassssiiee]

@mzaian is it OK now? Please advise...

[MrFreezeex]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbaassssiiee, MrFreezeex, yankay

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [yankay]

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