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 distribution detection logic #15

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Conversation

rtorrero
Copy link
Contributor

This PR adds:

  • A check in the pre_tasks of the playbook. The check will prevent the execution on distributions others than SLES for SAP 15 SP3, SP4 or SP5
  • A conditional that executes the expect workaround that manually installs the user/group for prometheus. This conditional will ensure that the workaround is only executed when needed (SP4 and SP5)
  • Changes yaml formatting for the playbook so that we use 2 spaces for indentation. I've seen this in most of YAML guidelines and my formatter follows this also. I can remove it if we prefer to stay with 1-space.

I've checked the logic in all 3 supported targets and it works as expected

@rtorrero rtorrero marked this pull request as ready for review January 12, 2024 16:38
@rtorrero rtorrero requested a review from EMaksy January 12, 2024 16:40
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.

Hey @rtorrero,
I guess that this approach is good enough if the differences are so small.
There are other strategies like having specific variables files per distro for more advanced use cases.
My unique concern again is the PR environment, which I think we start failing again.
What do you have in mind for that?

playbook.yml Outdated
@@ -3,79 +3,87 @@
- name: Install thirdparties
hosts: trento-server
become: true
pre_tasks:
- name: Check SLES distribution and version
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to check if we need to accept openSUSE leap as well.
Otherwise the PR environment workflow will start failing, as It is not based on SLES4SAP (but i'm not sure if it is based in Leap neither)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, let's discuss it today in the daily 👍

playbook.yml Outdated
- gcc
- python3-devel
- sudo
- name: Install installation prerequisites
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need some yaml linter here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would go for the double-space indenting that is used more extensively. Should we enforce this in the CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really care, as long as we don't do this "out of the blue" changes

roles/prometheus/tasks/main.yml Outdated Show resolved Hide resolved
roles/prometheus/tasks/main.yml Outdated Show resolved Hide resolved
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

Same comments as @arbulu89, also we need to support LEAP, for the pr env itself and also for Vagrant usage/testing

We can go with the latest leap version, changing the Vagrant box ofc

@rtorrero
Copy link
Contributor Author

@CDimonaco @arbulu89 changes applies, except for the linting. I would like to address that separately as we need to change more than a few existing files. I will do that next and add https://github.com/marketplace/actions/yaml-lint to the CI, wdyt?

@CDimonaco
Copy link
Member

@CDimonaco @arbulu89 changes applies, except for the linting. I would like to address that separately as we need to change more than a few existing files. I will do that next and add https://github.com/marketplace/actions/yaml-lint to the CI, wdyt?

Please check locally on the machine if yaml lint and ansible-lint conficts, ansible lint has already some rules on formatting the yaml files

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.

I would personally remove all the linting changes, and apply them once we have some linting rules

playbook.yml Outdated
- gcc
- python3-devel
- sudo
- name: Install installation prerequisites
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really care, as long as we don't do this "out of the blue" changes

@rtorrero rtorrero merged commit c8bc9f7 into main Jan 16, 2024
5 checks passed
@stefanotorresi stefanotorresi deleted the add-distribution-detection branch January 25, 2024 13:47
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