-
Notifications
You must be signed in to change notification settings - Fork 135
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
Migrate EST tests to ansible #4472
Conversation
I have followed the idea to create a role for each component to test but not sure if it is the best approach. For this first attempt I was more interested on understanding the use of ansible in this context. Compared to the original workflow I think the output is less clear but ansible allows to improve on that with debug tasks and other features. Additionally, I have built the pki-runner image using docker and then run the playbook without problem. |
1fecc73
to
5b98516
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice! I'm still looking at this, but here are my initial comments.
Is it possible to store these files under the tests
folder so that they will be included into pki-tests
package so they can be distributed & installed more easily?
Is there any way to make it easier to read the output (e.g. by printing the output or storing it in a file instead of displaying it in JSON format)? I think this is very important for dev and QE.
IIRC correctly this is the return format QE has for the tests in their repo - but it would appear there are a few options for doing it: https://docs.ansible.com/ansible/latest/reference_appendices/logging.html |
I think it looks very promising! The big question I have is whether it works nicely if you run it locally/in a scratch VM as that was one of the big benefits? Is that easy to do? |
Additionally, to the logging it is possible to modify the way output is shown with plugin and configuration (as example https://docs.ansible.com/ansible/latest/collections/community/general/unixy_callback.html) but I have not yet tested them so not sure yet. |
Sure, I'll move |
For the |
5b98516
to
96bfa19
Compare
I think the |
About the logging, I'm not sure how the output of QE tests looks like, but in GH tests the output shows a lot of information such as installation logs, systemd logs, content of NSS database, CLI output, etc. which is useful when creating the tests initially and for troubleshooting issues later. The current output is displayed in JSON format which makes it hard to read. |
96bfa19
to
157ecb5
Compare
I have modified the ansible configuration file to have a different output format. Still there is a lot JSON but now the output is more readable. |
e58d492
to
b781600
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, the output is looking a lot better now! It might be nice if we can separate the output from one step to another, but it's a lower priority.
I do have some comments, but there's no major issues. Feel free to update/merge. Thanks!
tests/ansible/ansible.cfg
Outdated
# (pathspec) Colon separated paths in which Ansible will search for Roles. | ||
;roles_path={{ ANSIBLE_HOME ~ "/roles:/usr/share/ansible/roles:/etc/ansible/roles" }} | ||
|
||
# (string) Set the main callback used to display Ansible output. You can only have one at a time. | ||
# You can have many other callbacks, but just one can be in charge of stdout. | ||
# See :ref:`callback_plugins` for a list of available options. | ||
stdout_callback=community.general.unixy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this file was copied from somewhere? Most of the params in this file are commented out. To keep it simple should we remove the unused params and just keep the ones we actually use like this one? We can put a link to the docs or to the original source in case we need to change other params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a generated file with ansible-config
command: https://docs.ansible.com/ansible/latest/reference_appendices/config.html#generating-a-sample-ansible-cfg-file. Since it is not copied and we use not other parameters we can just remove what is not used.
- name: Set up DS container | ||
community.docker.docker_container: | ||
name: "{{ ds_container }}" | ||
image: "{{ ds_image }}" | ||
hostname: "{{ ds_hostname }}" | ||
volumes: | ||
- /data | ||
- "{{ github_workspace }}:{{ shared_workspace }}" | ||
state: started | ||
detach: true | ||
env: | ||
DS_DM_PASSWORD={{ ds_password }} | ||
networks: | ||
- name: example | ||
aliases: | ||
- "{{ ds_hostname }}" | ||
ports: | ||
- 3389 | ||
- 3636 | ||
healthcheck: | ||
test: ["CMD", "dsctl", "slapd-localhost", "healthcheck"] | ||
start_period: 10s | ||
timeout: 10s | ||
interval: 15s | ||
retries: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the original GH workflow we use ds-container-create.sh
which can either create a DS container from quay.io/389ds/dirsrv
image or install a DS instance within a container created from pki-runner
image. The first method is probably simpler/faster, but the second method can be used to replicate what most people will actually be doing and also as a backup in case there's a problem with the DS image in quay.io (because IIUC it doesn't have the same level of support as the RPM). Is it possible to use this script in ansible? It's not required, but it would be nice if we have options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my initial idea was to implement in ansible what is done in the script so in the future we can drop it (and some others) but I am not sure it is the best approach. I left this decision to future PRs.
tests/ansible/est/tasks/main.yml
Outdated
container: "{{ pki_container }}" | ||
command: pki-server est-create | ||
|
||
- name: Configurte EST backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
tests/ansible/est/tasks/main.yml
Outdated
container_path: /etc/pki/pki-tomcat/est/backend.conf | ||
content: | | ||
class=org.dogtagpki.est.DogtagRABackend | ||
url=https://pki.example.com:8443 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be url=https://{{ pki_hostname }}:8443
?
tests/ansible/est/tasks/main.yml
Outdated
container: "{{ client_container }}" | ||
command: openssl x509 -in cacert.pem -noout -subject | ||
register: ca_subject | ||
failed_when: ('subject=O = EXAMPLE, OU = pki-tomcat, CN = CA Signing Certificate' not in ca_subject.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I'd prefer a more precise validation (i.e. using an exact match or a regex) rather than an in
operator to avoid false positives (e.g. unintentionally matching a longer string), but I'll let you decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have replaced stdout
with stdout_lines
(see here). In this case the in operator verify if the string on the left is one of the elements in the list so the match is not partial but on the full string.
b781600
to
ec51137
Compare
Tests based on GitHub workflow cannot easily be executed locally before the code is pushed making more difficult to identify problems during the development. Additionally, there are several limitation on how workflows can be combined. Moving to ansible should solve the limitation and allow the execution of test on developer machine. This is the first step aims at converting a single workflow to ansible. The preliminary steps of building the docker images and deploy onto the runner are still based on github actions. If/when all workflows will use ansible the overall CI activities could be reorganised to optimise the execution in the available runners.
ec51137
to
1a9b1c0
Compare
Kudos, SonarCloud Quality Gate passed! |
@edewata Thanks! |
Tests based on GitHub workflow cannot easily be executed locally before the code is pushed making more difficult to identify problems during the development.
Additionally, there are several limitation on how workflows can be combined.
Moving to ansible should solve the limitation and allow the execution of test on developer machine.
This is the first step aims at converting a single workflow to ansible.
The preliminary steps of building the docker images and deploy onto the runner are still based on github actions.
If/when all workflows will use ansible the overall CI activities could be reorganised to optimise the execution in the available runners.