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

CI: Switch to GitHub for ansible-lint #325

Closed
wants to merge 6 commits into from

Conversation

mamercad
Copy link
Contributor

@mamercad mamercad commented Dec 5, 2022

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.

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Dec 5, 2022
arm4b
arm4b previously requested changes Dec 5, 2022
.github/workflows/lint.yaml Show resolved Hide resolved
@arm4b arm4b dismissed their stale review December 5, 2022 18:34

The changes requested are not actual anymore due to ansible-st2 v3.2 released

@mamercad mamercad changed the title Switch to GH for ansible-lint CI: Switch to GitHub for ansible-lint Dec 22, 2022
@mamercad mamercad requested a review from arm4b December 31, 2022 18:20
@mamercad
Copy link
Contributor Author

@armab Is there anything left to do here?

.github/workflows/lint.yaml Outdated Show resolved Hide resolved
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

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?

@mamercad
Copy link
Contributor Author

mamercad commented Jan 3, 2023

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
Copy link
Member

arm4b commented Jan 3, 2023

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 🟢 status with a linter that doesn't work with the current ansible version/codebase with a 🔴 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
Copy link
Member

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
Copy link
Member

arm4b commented Jan 3, 2023

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
Copy link
Contributor Author

mamercad commented Jan 5, 2023

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

@mamercad mamercad closed this Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants