Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Refactor version checks #366

Merged

Conversation

aleksei-burlakov
Copy link
Contributor

Use ansible.builtin.package_facts instead of shell.

I think the PR is self explaining. There is still a minor improvement that should be done:
There is a repeating block

- name: Gather the package facts
  ansible.builtin.package_facts:
    manager: auto

in the checks 2.2.3 ... 2.2.7. However if I put this block in the runner/ansible/roles/load_facts/tasks/main.yml like here https://github.com/trento-project/trento/pull/344/files#diff-1c2ed1309d3811de445241b2d87823d1e5b0df3e383c4d6a4f7a39996dacf107, the trento-runner container simply fails to start.

@arbulu89 @brett060102 what do you think about this?

@aleksei-burlakov aleksei-burlakov added enhancement Improvement of existing features runner Related to the Trento Runner component labels Oct 21, 2021
Copy link
Contributor

@brett060102 brett060102 left a comment

Choose a reason for hiding this comment

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

Code does work, but would suggest following change:
1: do package_facts one in load facts:

index 581d485..569e00d 100644
--- a/runner/ansible/roles/checks/2.2.3/tasks/main.yml
+++ b/runner/ansible/roles/checks/2.2.3/tasks/main.yml
@@ -1,11 +1,7 @@
 ---
 
-- name: Gather the package facts
-  ansible.builtin.package_facts:
-    manager: auto
-
 - block:
-    - name: Post results
+    - name: "{{ name }} Post results"
       import_role:
         name: post-results
   vars:
diff --git a/runner/ansible/roles/checks/2.2.4/tasks/main.yml b/runner/ansible/roles/checks/2.2.4/tasks/main.yml
index a0695c2..cf0e702 100644

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@aleksei-burlakov
Nice! It looks simply much better.
We need to change the comparisons to >= as the expected value is accepted as well.

Besides that, I'm a bit concerned about the repeated usage of the package loading task. If the execution of each task is executed completely, and it takes a lot of time, I would vote to moving it to some generic role.

runner/ansible/roles/checks/2.2.3/tasks/main.yml Outdated Show resolved Hide resolved
runner/ansible/roles/checks/2.2.3/tasks/main.yml Outdated Show resolved Hide resolved
runner/ansible/roles/checks/2.2.4/tasks/main.yml Outdated Show resolved Hide resolved
Use ansible.builtin.package_facts instead of shell
@aleksei-burlakov aleksei-burlakov force-pushed the refactor-version-checks branch 2 times, most recently from 8400670 to f2be57d Compare October 26, 2021 11:36
@aleksei-burlakov
Copy link
Contributor Author

@arbulu89 @brett060102 could you please approve the PR?

@@ -11,7 +11,7 @@ RUN make build
FROM python:3.7-slim AS trento-runner
RUN ln -s /usr/local/bin/python /usr/bin/python \
&& /usr/bin/python -m venv /venv \
&& /venv/bin/pip install 'ansible~=4.6.0' 'ara~=1.5.7' \
&& /venv/bin/pip install 'ansible~=4.6.0' 'ara~=1.5.7' 'rpm~=0.0.2' \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The python-rpm has other versioning that the suse rpm rpm. It's not 4.14.02 but 0.0.2

Copy link
Contributor

Choose a reason for hiding this comment

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

mmm, the version 0.0.2 looks a like a placeholder. Maybe we should install using apt-get install python3-rpm which would bring the a more maintained version.
Anyway, if this works it is fine for me

Copy link
Contributor

@brett060102 brett060102 left a comment

Choose a reason for hiding this comment

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

LGTM and tests cleanly for me.

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Thanks @aleksei-burlakov ,
It looks good to me. I have some doubts with the 0.0.2 rpm version, but if it works I guess we can move forwatd

Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

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

I like how much simpler this looks now, gj!

@aleksei-burlakov aleksei-burlakov merged commit 9015178 into trento-project:main Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement of existing features runner Related to the Trento Runner component
Development

Successfully merging this pull request may close these issues.

None yet

4 participants