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

add ansible/lint #8

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

add ansible/lint #8

wants to merge 8 commits into from

Conversation

artcz
Copy link
Contributor

@artcz artcz commented Jan 17, 2025

No description provided.

@artcz artcz self-assigned this Jan 17, 2025
@artcz
Copy link
Contributor Author

artcz commented Jan 17, 2025

Known issue: docker-compose commands in 03_app are still triggering lint errors. Other errors/warnings are fixed.

Fixed after merging separate PR and rebasing this branch.

Makefile Outdated
@@ -126,3 +127,8 @@ deploy/provision:
deploy/app:
@echo "Deploying version $(V) to a remote server"
$(DEPLOY_CMD) playbooks/03_app.yml --extra-vars "app_version=$(V)"

deploy/lint:
Copy link

Choose a reason for hiding this comment

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

Why? Liniting should happen before the files get deployed?
See: https://blog.42mate.com/installing-and-using-pre-commit-with-ansible-lint/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or before they get pushed, yes. The deploy/ here is to separate from make lint which lints the source/app.
Another option would be to make it lint/deploy target or something like that. WDYT?

Copy link

Choose a reason for hiding this comment

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

Aehm yes. Before push. Or at least before merge.
So either it is just "lint" if you want to execute manually. But I will forget to do this. Or you add a pre-commit hook to it. Like at website.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and the next step would be to add it here: https://github.com/EuroPython/internal-bot/pull/3/files :)

It currently only runs tests, but the goal is to also run linter as part of the same checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed after rebasing on the new CI/CD setup. Now lint/deploy is run as part of build

deploy/playbooks/02_nginx.yml Outdated Show resolved Hide resolved
deploy/playbooks/02_nginx.yml Outdated Show resolved Hide resolved
deploy/playbooks/02_nginx.yml Outdated Show resolved Hide resolved
deploy/playbooks/03_app.yml Outdated Show resolved Hide resolved
deploy/playbooks/03_app.yml Outdated Show resolved Hide resolved
deploy/playbooks/03_app.yml Outdated Show resolved Hide resolved
deploy/playbooks/03_app.yml Outdated Show resolved Hide resolved
deploy/playbooks/03_app.yml Outdated Show resolved Hide resolved
deploy/playbooks/03_app.yml Outdated Show resolved Hide resolved

- name: Restart everything and finish
shell: "docker compose up -d"
ansible.builtin.command:
Copy link

Choose a reason for hiding this comment

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

Me not likey ansible.builtin.cmd. You could use community.docker.docker_compose_v2
E.g. https://github.com/EuroPython/infra/blob/53c2fa4d997929a095fa4b10144baa0a34b05ce7/playbooks/roles/zammad/tasks/main.yaml#L49

Comment on lines +72 to +73
changed_when: output.rc != 0
failed_when: output.rc != 0
Copy link

Choose a reason for hiding this comment

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

This does not make sense. How should ansible know if this has failed or changed if the conditional is the same?
For ansible:
changed = continue
failed = stop here

Comment on lines +64 to +65
changed_when: output.rc != 0
failed_when: output.rc != 0
Copy link

Choose a reason for hiding this comment

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

This does not make sense. How should ansible know if this has failed or changed if the conditional is the same?
For ansible:
changed = continue
failed = stop here

Comment on lines +56 to +57
changed_when: output.rc != 0
failed_when: output.rc != 0
Copy link

Choose a reason for hiding this comment

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

This does not make sense. How should ansible know if this has failed or changed if the conditional is the same?
For ansible:
changed = continue
failed = stop here

Comment on lines +19 to +20
changed_when: output.rc != 0
failed_when: output.rc != 0
Copy link

Choose a reason for hiding this comment

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

This does not make sense. How should ansible know if this has failed or changed if the conditional is the same?
For ansible:
changed = continue
failed = stop here

Comment on lines +53 to +54
chdir: "{{ ansible_user_dir }}"
cmd: "docker compose up -d"
Copy link

Choose a reason for hiding this comment

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

Me not likey ansible.builtin.cmd. You could use community.docker.docker_compose_v2
E.g. https://github.com/EuroPython/infra/blob/53c2fa4d997929a095fa4b10144baa0a34b05ce7/playbooks/roles/zammad/tasks/main.yaml#L49

@@ -15,6 +15,7 @@ CI_RUN=cd intbot && DJANGO_SETTINGS_MODULE="intbot.settings" DJANGO_ENV="ci"

# Deployment
DEPLOY_CMD=cd deploy && uvx --from "ansible-core" ansible-playbook -i hosts.yml
DEPLOY_LINT_CMD=cd deploy && uvx --from "ansible-lint" ansible-lint
Copy link

Choose a reason for hiding this comment

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

I already commented on that in chat:
Lint should happen before we check-in . Or before we merge the latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants