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 service tests to the logging job #153

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

myadla
Copy link
Contributor

@myadla myadla commented Sep 27, 2024

mgirgisf and others added 2 commits September 3, 2024 11:22
Add service tests to the logging job

adding the tasks from #123 into a spearate PR
@myadla myadla marked this pull request as draft September 27, 2024 20:54
Copy link
Collaborator

@elfiesmelfie elfiesmelfie left a comment

Choose a reason for hiding this comment

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

As well as the comments inline, can you update the zuul job to run this? (based on #149)

@@ -0,0 +1,13 @@
---
- name: "[TEST] verify container status"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The TEST prefix isn't chosen yet, so this may change

Copy link
Collaborator

Choose a reason for hiding this comment

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

The prefix will be "TEST"

ansible.builtin.shell:
cmd: |
podman ps -a --format "{{ '{{.Names}} {{.Status}}' }}" | grep "{{ container_name }}" | awk '{print $2}'
changed_when: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a failed_when, if you're going to mark it with a TEST prefix, otherwise, it'll pass as long as the shell cmd returns something.

Alternatively, don't mark this task as a test, but mark the following task as a test.

Currently, we rely on the custom_logger to produce a file that gathers the test-ids=passed|failed output, so there should be a test ID in the second line of the task name.
Hopefully, we can remove this at some point soon, and add a check to report_result that checks the junit output instead.

Comment on lines +9 to +13
- name: Fail if container is not 'Up'
ansible.builtin.fail:
msg: "Container '{{ container_name }}' is not in 'Up' status. Current status: {{ container_status.stdout }}"
when:
- "'Up' not in container_status.stdout"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this, it makes it clear why the test failed.

However, this task should be the one one with TEST in the name.

Requirements
------------

None
Copy link
Collaborator

Choose a reason for hiding this comment

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

podman is required

Role Variables
--------------
Variable required for all tasks to run:

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should only be adding the container tests

@@ -0,0 +1,13 @@
---
- name: "[TEST] verify container status"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The prefix will be "TEST"

vars:
container_name: "{{ item }}"

---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
---

Comment on lines +2 to +11
# All containers need to be up in order for this task to pass
- name: "[TEST] verify status of multiple containers"
ansible.builtin.include_tasks:
file: container_status.yml
loop: "{{ containers }}"
loop_control:
label: "{{ item }}"
vars:
container_name: "{{ item }}"

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is separate from adding the service checks.

It should NOT be run unconditionally

container_name: "{{ item }}"

---
- name: "[TEST] Verify service for logging"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a conditional here, so that the service_tests.yml will only be included when the right vars are defined

roles/common/tasks/service_tests.yml Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants