StackStorm / ansible-st2

Ansible Roles and Playbooks to deploy StackStorm
https://galaxy.ansible.com/StackStorm/stackstorm/
Apache License 2.0
100 stars 75 forks source link

CI: Switch to GitHub for `ansible-lint` #325

Closed mamercad closed 1 year ago

mamercad commented 1 year ago

Switch to GitHub Workflows for Ansible linting (which performs YAML linting as well). It can't hurt to lint on pull request but linting daily isn't really necessary (since, ideally, linting problems will be fixed before being merged into master). The workflow annotations are kind of nice looking.

mamercad commented 1 year ago

@armab Is there anything left to do here?

mamercad commented 1 year ago

The latest ansible-lint CI is failing 🔴 now. So it depends on your plan and how you'd like to move forward with all the changes.

Do we replace CircleCI -> GH Actions with the same config and pinned ansible-lint and continue the rest in other PR, or do you want to continue the work for every Ansible lint warning here? That might be lots of changes.

🤔 Is there a way to pin ansible-lint, similar to how we pin the supported Ansible version?

Unfortunately, it doesn't look like there's a way to couple Ansible and Ansible Lint ... moreover, the Ansible Lint Action is very spartan as far as configuration goes. I think that'd I'd rather not combine "switching linting from Circle to GitHub" and "fixing all the linting problems" in the same pull request. The fact of the matter is, there are quite a few linting problems right now (uncovered), yet overall, the roles and playbooks are working just fine. That being said, I think it's okay to merge this without fixing everything, too.

arm4b commented 1 year ago

It's failing because the latest ansible-lint relies on the latest ansible version and syntax. Because we pinned Ansible version for this repo, we also pinned the linter version that's associated with that Ansible core version.

That being said, I think it's okay to merge this without fixing everything, too.

That way we're replacing the ansible-lint that is working for the currently pinned ansible version and has a :green_circle: status with a linter that doesn't work with the current ansible version/codebase with a :red_circle: status.

I can't merge the broken build to the main branch as it'll be a regression for the repository. We need to find an ansible-lint image that allows pinning the version so we can associate it with the Ansible version we support (now and in the future as we upgrade).

cognifloyd commented 1 year ago

ansible-lint is designed to work separately from the version of Ansible you use to run the playbook. So Ansible should be in one venv, and ansible-lint should be in another.

It may be beneficial to hold off on updating ansible-lint. I've been working on a feature for it (ansible-lint --write) that allows it to update (transform) playbooks / roles to fix identified issues automatically (where feasible). I don't have an ETA though.

Source of knowledge: I'm also an ansible-lint maintainer thanks to all the refactoring work I've done for that --write feature.

arm4b commented 1 year ago

I believe some of those are relevant to the outdated syntax, like

Use `ansible.builtin.fail` or `ansible.legacy.fail` instead.
Role loop_var should use configured prefix.> while processing roles/StackStorm.nodejs/tasks/main.yml (tasks): Unable to convert role name 'StackStorm.nodejs' to valid variable name.
schema[meta]: Additional properties are not allowed ('categories' was unexpected)

Thanks, @cognifloyd! Sounds like it's a good testing opportunity for the --write feature as we have hundreds of warning messages from the latest ansible-lint.

mamercad commented 1 year ago

Closed in favor of ansible-lint --write down the road.