Skip to content

Adds ansible-lint GHA step in lint workflow #1281

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Stringy
Copy link
Collaborator

@Stringy Stringy commented Aug 10, 2023

Description

Adds ansible-lint to CI to improve our ansible quality, especially now that our playbooks and roles are expanding for multi-arch.

This PR will also fix any lint errors that are introduced by this addition.

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Copy link
Collaborator

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

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

LGTM!

More linting is always welcomed IMO!

Comment on lines +96 to +128
- "Pushed the following images:"
- " {{ collector_image }}-{{ arch }}-slim"
- " {{ collector_image }}-{{ arch }}-base"
- " {{ ansible_env.RHACS_ENG_IMAGE }}-{{ arch }}-slim"
- " {{ ansible_env.RHACS_ENG_IMAGE }}-{{ arch }}-base"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dang, I prefer arrays to be in the same line for the object, but OK.

when: not python_exists
- when: not python_exists
creates: /usr/bin/python3
block:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

delay: 10
sleep: 10
timeout: 60
- name: Wait for reboot to complete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be extra clear, we don't need to reboot if the interpreter was already installed, which we were doing before, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah we were always rebooting before, but that only needs to happen when we've attempted to install a package (it's not applied until the machine reboots) so now it's more precise in its behaviour

@@ -3,11 +3,18 @@
set_fact:
distro: "centos"

- name: Install yum utils
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wonder what lint error triggered this code block to suddenly appear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there's a rule that analyses scripts (shell and command blocks) for commands that have a pre-existing ansible task. Since we were manually using yum in the shell below, it was flagging on that. It's also why the use of make has now changed in some other the other playbooks

@github-actions
Copy link

github-actions bot commented Aug 10, 2023

Kernel Method Without Collector Time (secs) With Collector Time (secs) Baseline median (secs) Collector median (secs) PValue
rhel.rhel-8 ebpf 213.819 226.519 204.92 213.7 🟢
ubuntu-os.ubuntu-2004-lts ebpf 227.219 235.271 236.1 234.97 🟢
ubuntu-os.ubuntu-2204-lts ebpf 243.798 249.929 222.56 229.05 🟢

@Stringy Stringy force-pushed the giles/ansible-lint-integration branch from 5e398ef to 615945e Compare August 21, 2023 08:52
@Stringy Stringy force-pushed the giles/ansible-lint-integration branch from bf1aec4 to 3d8efde Compare September 2, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants