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

Fix OS test check and update supported OSes configuration #559

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

morgan-patou
Copy link
Contributor

@morgan-patou morgan-patou commented Mar 17, 2023

Description:
Task with name "- name: Compare host OS with supported matrix" is currently being skipped most of the time. This is because "skip_os_test" is undefined and because "ansible_distribution_version | int not in os_versions" is failing as well. The transformation to "INT" means that only the major version is kept (E.g.: RedHat 8.6 >> 8). Since "8" is never present in "os_versions" (which contains 8.2, 8.5, 8.6, etc.), then this check is always skipped. According to the documentation, it is supposed to be the opposite, meaning that it's always executed, except if "-i skip_os_test=true" is added as extra var.

Also, the correct ansible distribution for RedHat is "RedHat" and not "RHEL". This causes the "supported_os" to not find anything in case the target OS is RedHat, c.f. https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_conditionals.html#ansible-facts-distribution.

Solution:
This PR includes the switch from "RHEL" to "RedHat" in the group_vars/all.yml file as well as the fix of the conditions so that the check is always executed and gives the expected result, meaning:

  1. failure if distribution/version doesn't match what is defined in the group_vars/all.yml file
  2. skipping if distribution/version matches OR if skip_os_test=true

Please review

@CLAassistant
Copy link

CLAassistant commented Mar 17, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@alxgomz alxgomz left a comment

Choose a reason for hiding this comment

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

Well spotted! Thanks for the contribution. Just one small suggestion, but that's all OK to move it forward.

playbooks/acs.yml Outdated Show resolved Hide resolved
@morgan-patou
Copy link
Contributor Author

Works for me with this alternative definition. I quickly tested the different possibilities (supported, unsupported, without and with false/true for the skip_os_test), no problems spotted.

Copy link
Member

@gionn gionn left a comment

Choose a reason for hiding this comment

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

There are a few test failures that expose that we missed updating the supported os matrix due to the bug you just fixed.

Rocky 8.6 and 8.7 should be added for sure, not sure about CentOS 7.7 that is an old version and I suppose we need to bump the AMI we are testing against.

up to you if you want to try to tackle also this last issues in order to obtain a green build, or just put this PR on hold and I can try to take a look soon and raise another PR to fix those tests (internally tracked as OPSEXP-2047).

@morgan-patou
Copy link
Contributor Author

morgan-patou commented Mar 22, 2023

There are a few test failures that expose that we missed updating the supported os matrix due to the bug you just fixed.

Rocky 8.6 and 8.7 should be added for sure, not sure about CentOS 7.7 that is an old version and I suppose we need to bump the AMI we are testing against.

up to you if you want to try to tackle also this last issues in order to obtain a green build, or just put this PR on hold and I can try to take a look soon and raise another PR to fix those tests (internally tracked as OPSEXP-2047).

If needed I can modify the group_vars/all.yml to include Rocky and CentOS versions mentioned but I do not know if you have any other steps to perform to include a new OS? Do you have some guidelines with the list of all steps? I would also need some details on which ACS versions are supported on these 2 OS, so I can update the doc as well. Currently CentOS 7.7 is marked as supported for all ACS versions from 6.2 to 7.3, including Community as well.

@gionn
Copy link
Member

gionn commented Mar 22, 2023

If needed I can modify the group_vars/all.yml to include Rocky and CentOS versions mentioned but I do not know if you have any other steps to perform to include a new OS?

You should just really need to add Rocky 8.6, Rocky 8.7, CentOS 7.7

docs should be fine https://github.com/Alfresco/alfresco-ansible-deployment/blob/master/docs/README.md#versioning

thanks again!

Update the group_vars/all.yml file to add Rocky 8.6 / 8.7 as well as CentOS 7.7 in the supported OS
@morgan-patou
Copy link
Contributor Author

If needed I can modify the group_vars/all.yml to include Rocky and CentOS versions mentioned but I do not know if you have any other steps to perform to include a new OS?

You should just really need to add Rocky 8.6, Rocky 8.7, CentOS 7.7

docs should be fine https://github.com/Alfresco/alfresco-ansible-deployment/blob/master/docs/README.md#versioning

thanks again!

I updated the PR with morgan-patou@2804a9d but now there is one fail on ACS7.3/Ubuntu20.04, which shouldn't be linked to this PR since it was working before I believe. Is it possible to rerun a specific failed test? I don't see any actions on my side but maybe I'm missing some permissions to be allowed to do that.

@gionn gionn requested a review from alxgomz March 23, 2023 07:51
@gionn gionn changed the title Fix OS test condition and RedHat ansible distribution name Fix OS test check and update supported OSes configuration Mar 23, 2023
Copy link
Contributor

@alxgomz alxgomz left a comment

Choose a reason for hiding this comment

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

OK to merge as-is for now but we'll need another task to update our centos AMI and drop the support for centos 7.7 as most ACS version actually specify 7.9

@gionn
Copy link
Member

gionn commented Mar 23, 2023

Thanks @morgan-patou!

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.

4 participants