community.postgresql
community.postgresql copied to clipboard
ansible-collections
Add ansible-test integration for GHA
SUMMARY
Like for the sanity tests, same external deps.
ISSUE TYPE
- Feature Pull Request
https://github.com/ansible-collections/community.postgresql/issues/154
COMPONENT NAME
docker testing with GHA
ADDITIONAL INFORMATION
Only pg14 at the moment. Other versions may fail at the moment.
@klando thanks for working on this!
Could you please take a look at the errors.
Several questions:
- Would it make sense to have all CI related files in this repo?
- I don't see Ansible branches mentioned anywhere (besides stable-2.11 specified in webknjaz's repo). The collection has to be tested (all kinds of tests, i.e. integration, units, sanity) against all supported Ansible versions (currently branches are stable-2.9, stable-2.10, stable-2.11, stable-2.12, devel). See AZP jobs as an example, they report, e.g. 2.11, 2.12, etc.).
- Could you please explain how this works? I looks much more complex than in c.mysql, for example. I'm not sure i'll able to sort this out quickly when needed in the future:) Is this really necessary? Should we keep things as simple as possible (preferable in one file, without "encapsulation" and not really necessary variables, etc.? As it looks much simpler in c.mysql (and in many other collections) and it works there, it should work here.
@klando thanks for working on this!
Could you please take a look at the errors.
Its due to the behavior of pull request, I've based on examples from dorny filter but it's wrong. will fix
Several questions:
1. Would it make sense to have all CI related files in this repo?
It's possible. I don't know the policy regarding external tools in CI for Ansible.
2. I don't see Ansible branches mentioned anywhere (besides stable-2.11 specified in webknjaz's repo). The collection has to be tested (all kinds of tests, i.e. integration, units, sanity) against all supported Ansible versions (currently branches are stable-2.9, stable-2.10, stable-2.11, stable-2.12, devel). See AZP jobs as an example, they report, e.g. 2.11, 2.12, etc.).
They are, see here for expected results: https://github.com/klando/community.postgresql/actions/runs/1491750992
3. Could you please explain how this works? I looks much more complex than in c.mysql, for example. I'm not sure i'll able to sort this out quickly when needed in the future:) Is this really necessary? Should we keep things as simple as possible (preferable in one file, without "encapsulation" and not really necessary variables, etc.? As it looks much simpler in c.mysql (and in many other collections) and it works there, it should work here.
It's the simplest solution I found for collection maintainers as the required edition on the collection is very limited. I can update the README(s) accordingly.
First, the matrix is built from variables provided by an action in charge of providing the inclusion/exclusion and python version and so on. Very useful IMO. Note that Ansible may expose the python versions, stables branches via endpoints and it would me much more easier for everyone, the docker image can be parsed from the ansible-test source, I was just lazy doing that on the first version. Note that they are 2 possible matrix: 1 one for testing python versions, one for testing with distinct docker images.
There is an action to look up the list of modules and building the tests list according to the files changed, files change are detected by an action using those lists.
And the test are run with the last action, from ansible-community.
Codecov Report
Merging #166 (4bc867f) into main (44dc453) will decrease coverage by
0.26%. The diff coverage isn/a.
@@ Coverage Diff @@
## main #166 +/- ##
==========================================
- Coverage 81.16% 80.90% -0.27%
==========================================
Files 34 24 -10
Lines 4636 4200 -436
Branches 1048 1001 -47
==========================================
- Hits 3763 3398 -365
+ Misses 594 481 -113
- Partials 279 321 +42
| Flag | Coverage Δ | |
|---|---|---|
| integration | 80.90% <ø> (?) |
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 44dc453...4bc867f. Read the comment docs.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Two general points:
1. The matrix (https://github.com/ansible-collections/community.postgresql/actions/runs/1494096774) is somewhat confusing. What exactly are the `plugins`, `changes` and `license` jobs doing, and what are they needed for? Also there are `test-python` boxes which are skipped, is that to be expected / normal?
Yeah, I should rename.
-
plugins is related to inspection of plugins provided by the collection in order to build a filter list for files related to test target. It allows to trigger testing of only the modules which has changed (not the full stack of modules which run for up to 40 minutes).
-
changes is the action to evaluate changes files and set up relevant variables accordingly (test target for instance). It's here that we detect a change in module A thus add a
ansible-test integration Ain the job. -
license is here because I didn't know where to add it. GPL 3 suggests to show on terminal or an "about" box the copyright, I added it here because it is related to the workflow (action themselves output the © in their output). Its just for being compliant but it is maybe too much ...
2. I don't think that the collection's CI should depend this critically on third-party repositories; it would probably be best if the workflow templates could be moved to a repository in gh.com/ansible-collections or gh.com/ansible-community.
LGTM (to move in one or the other organization).
3. It is probably a good idea (both from environmental impact and RedHat's CI budget) to limit the number of combinations, so that not every combination of ansible-core version, Python version and postgresql version is combined.
I am not enough involved in ansible requirements to address that. I tend to agree, at least for the environment impact (but remember that the current run is the largest one possible, and often just a subset of the ansible-test will be required).
On the number of jobs, it's easy to change. Either in the workflow when calling the action, or (I prefer) in the action having a flag for full-matrix or eco-matrix (the most important and interesting versions with the smallest matrix).
I found disturbing the very large number of versions to support, even old dead and unmaintained like python 2.something, the action to get the versions was here to at least give developers a reference of the largest matrix they are supposed to handle in the plugins. but getting the minimal one is of interest too !
About PostgreSQL version, I fear it is of interest to keep all stable versions in the matrix. There is work TBD on integration test to make them less heavy, and also adding roles to the collection to manage PostgreSQL. At this point it will very important to test against all combination and on several distros (at least debian, ubuntu, redhat, centos, and probably often 2 versions of each). Carefully limiting the tests triggered by changes looks even more important here.
I'll appreciate suggestions on an expected eco-matrix content (not sure which versions qualify more than other ones, for both sanity and integration tests)...
Note that the matrix will be lifted in May 2022, and I suppose ansible does not expect to maintain several stable versions at the same time any more...
* plugins is related to inspection of plugins provided by the collection in order to build a filter list for files related to test target. It allows to trigger testing of only the modules which has changed (not the full stack of modules which run for up to 40 minutes). * changes is the action to evaluate changes files and set up relevant variables accordingly (test target for instance). It's here that we detect a change in module A thus add a `ansible-test integration A` in the job.
Just a quick question: why not use ansible-test's change detection? It already has support for this.
Note that the matrix will be lifted in May 2022, and I suppose ansible does not expect to maintain several stable versions at the same time any more...
This is news to me. I think community collections will always support multiple stable branches at once. Do you have any information on why this should/will change?
* plugins is related to inspection of plugins provided by the collection in order to build a filter list for files related to test target. It allows to trigger testing of only the modules which has changed (not the full stack of modules which run for up to 40 minutes). * changes is the action to evaluate changes files and set up relevant variables accordingly (test target for instance). It's here that we detect a change in module A thus add a `ansible-test integration A` in the job.Just a quick question: why not use ansible-test's change detection? It already has support for this.
no idea it was possible nor how to to do it.
Note that the matrix will be lifted in May 2022, and I suppose ansible does not expect to maintain several stable versions at the same time any more...
This is news to me. I think community collections will always support multiple stable branches at once. Do you have any information on why this should/will change?
I'm puzzled, I read that on the ansible doc: https://docs.ansible.com/ansible/devel/reference_appendices/release_and_maintenance.html#ansible-core-changelogs 2.9-2.11 to become EOL in May 2022, remains only 2.12 and devel...
Just a quick question: why not use ansible-test's change detection? It already has support for this.
no idea it was possible nor how to to do it.
You have to pass --changed --base-branch main (if the PR is against main). This works for all three test types. You need to make sure that you have a full enough fork though, and not just a shallow checkout.
Note that the matrix will be lifted in May 2022, and I suppose ansible does not expect to maintain several stable versions at the same time any more...
This is news to me. I think community collections will always support multiple stable branches at once. Do you have any information on why this should/will change?
I'm puzzled, I read that on the ansible doc: https://docs.ansible.com/ansible/devel/reference_appendices/release_and_maintenance.html#ansible-core-changelogs 2.9-2.11 to become EOL in Nov 2022, remains only 2.12 and devel...
Well, ansible-core 2.13 is expected to be released in May 2022, and ansible-core 2.14 probably in November 2022. So basically once a stable branch is removed, another one gets added (except next spring, then two are removed and one added).
It depends also on the collection's release cycle, though, since new stable branches get added basically once they are released, while branches can only be removed on major releases. Also which older stable branches are supported by a collection depends on that collection. Collections can potentially keep supporting everything down to stable-2.9 for a longer time.
* license is here because I didn't know where to add it. GPL 3 suggests to show on terminal or an "about" box the copyright, I added it here because it is related to the workflow (action themselves output the © in their output). Its just for being compliant but it is maybe too much ...
I wonder whether it really needs to be printed. Most CLI programs licensed under GPL also only print it when you explicitly ask them (calling with --version), and sometimes even don't mention it in that case.
3. It is probably a good idea (both from environmental impact and RedHat's CI budget) to limit the number of combinations, so that not every combination of ansible-core version, Python version and postgresql version is combined.I am not enough involved in ansible requirements to address that. I tend to agree, at least for the environment impact (but remember that the current run is the largest one possible, and often just a subset of the ansible-test will be required). On the number of jobs, it's easy to change. Either in the workflow when calling the action, or (I prefer) in the action having a flag for
full-matrixoreco-matrix(the most important and interesting versions with the smallest matrix). I found disturbing the very large number of versions to support, even old dead and unmaintained like python 2.something, the action to get the versions was here to at least give developers a reference of the largest matrix they are supposed to handle in the plugins. but getting the minimal one is of interest too !
I'm not sure how this can be properly automated. Picking which combinations are useful is something the collection maintainers are most qualified for, and this is IMO one of the reasons why constructing the matrix should be part of the collection's workflow.
About PostgreSQL version, I fear it is of interest to keep all stable versions in the matrix.
That's totally fine. It's just not necessary to test every PostgreSQL version with every other ansible-core / Python combination. For older PostgreSQL versions, it suffices if they are tested with exactly one ansible-core / Python version combination, or maybe two (maybe one Python 2 and one Python 3, each with a different ansible-core version?).
I suggest making things as simple as possible so that future contributors can quickly figure out how to adjust CI when needed. Also I suggest avoiding all unnecessary things (like licensing) and limiting the combinations as Felix mentioned. "Simplicity is a feature." and it would be nice not to wait for tests longer than 5-10 minutes max:)
I suggest making things as simple as possible so that future contributors can quickly figure out how to adjust CI when needed. Also I suggest avoiding all unnecessary things (like licensing) and limiting the combinations as Felix mentioned. "Simplicity is a feature." and it would be nice not to wait for tests longer than 5-10 minutes max:)
Agreed, here is the current integration testing (all of them in a row) takes around 30 minutes, for a single version of postgresql on single ansible, on single python. Independently of this proposed GHA workflow.
Can we simplify the solution drastically anyhow? The current testing with AZP is far from being ideal but it looks easier to sort out. I would avoid chains of files, etc., i.e. IMO the simpler but not ideal solutions is better than great but complex. I don't mind to figure out what's happening there but people come and go and i worry mostly about future contributors / maintainer.
Is it a big challenge to implement the GHA matrix as in many collections, e.g. like in https://github.com/ansible-collections/community.dns/blob/main/.github/workflows/ansible-test.yml ? Looks pretty straightforward, doesn't it?
Is it a big challenge to implement the GHA matrix as in many collections, e.g. like in https://github.com/ansible-collections/community.dns/blob/main/.github/workflows/ansible-test.yml ? Looks pretty straightforward, doesn't it?
Really ? Maintaining all over the place the special cases. up to you.
exclude:
# Because ansible-test doesn't support python3.10 for ansible-core 2.11
- ansible: stable-2.11
python: "3.10"
Is it a big challenge to implement the GHA matrix as in many collections, e.g. like in https://github.com/ansible-collections/community.dns/blob/main/.github/workflows/ansible-test.yml ? Looks pretty straightforward, doesn't it?
Really ? Maintaining all over the place the special cases. up to you.
exclude: # Because ansible-test doesn't support python3.10 for ansible-core 2.11 - ansible: stable-2.11 python: "3.10"
Not a big issue for me. As i said, not ideal but simple solution can be better than complex. Plus having external dependencies can lead to red color in our CI one day
Is it a big challenge to implement the GHA matrix as in many collections, e.g. like in https://github.com/ansible-collections/community.dns/blob/main/.github/workflows/ansible-test.yml ? Looks pretty straightforward, doesn't it?
Really ? Maintaining all over the place the special cases. up to you.
exclude: # Because ansible-test doesn't support python3.10 for ansible-core 2.11 - ansible: stable-2.11 python: "3.10"
Well, that's basically what every collection will have to do anyway. Since a) you never want the full matrix, and b) what exactly is in the matrix usually depends a lot on the specific collection.
I only can see how this can get optimized away for specific classes of collections, like a set of cloud/REST API collections which do not use OS-specific containers, and which all support the same set of Ansible/-core/-base and Python versions.
Is it a big challenge to implement the GHA matrix as in many collections, e.g. like in https://github.com/ansible-collections/community.dns/blob/main/.github/workflows/ansible-test.yml ? Looks pretty straightforward, doesn't it?
in your example:
sanity:
name: Sanity (Ⓐ${{ matrix.ansible }})
strategy:
matrix:
ansible:
# It's important that Sanity is tested against all stable-X.Y branches
# Testing against `devel` may fail as new tests are added.
- stable-2.9
- stable-2.10
- stable-2.11
- stable-2.12
- devel
runs-on: ubuntu-latest
steps:
# ansible-test requires the collection to be in a directory in the form
# .../ansible_collections/${{env.NAMESPACE}}/${{env.COLLECTION_NAME}}/
- name: Check out code
uses: actions/checkout@v2
with:
path: ansible_collections/${{env.NAMESPACE}}/${{env.COLLECTION_NAME}}
- name: Set up Python
uses: actions/setup-python@v2
with:
# it is just required to run that once as "ansible-test sanity" in the docker image
# will run on all python versions it supports.
python-version: 3.8
# Install the head of the given branch (devel, stable-2.10)
- name: Install ansible-core (${{ matrix.ansible }})
run: pip install https://github.com/ansible/ansible/archive/${{ matrix.ansible }}.tar.gz --disable-pip-version-check
# run ansible-test sanity inside of Docker.
# The docker container has all the pinned dependencies that are required
# and all python versions ansible supports.
- name: Run sanity tests
run: ansible-test sanity --docker -v --color --coverage
working-directory: ./ansible_collections/${{env.NAMESPACE}}/${{env.COLLECTION_NAME}}
# ansible-test support producing code coverage date
- name: Generate coverage report
run: ansible-test coverage xml -v --requirements --group-by command --group-by version
working-directory: ./ansible_collections/${{env.NAMESPACE}}/${{env.COLLECTION_NAME}}
# See the reports at https://codecov.io/gh/ansible-collections/community.dns
- uses: codecov/codecov-action@v2
with:
fail_ci_if_error: false
can be replaced by:
sanity:
uses: data-bene/ansible-collection-gh-workflow/.github/workflows/workflow-sanity.yml@devel
Except that now, the collection has no control over which ansible-core stable branches are used for sanity tests, and changes to data-bene/ansible-collection-gh-workflow/.github/workflows/workflow-sanity.yml@devel (like adding a new stable branch) could randomly break the collection's CI; while other changes, such as removing a stable branch in the reusable workflow, can introduce bugs.
Except that now, the collection has no control over which ansible-core stable branches are used for sanity tests, and changes to
data-bene/ansible-collection-gh-workflow/.github/workflows/workflow-sanity.yml@devel(like adding a new stable branch) could randomly break the collection's CI; while other changes, such as removing a stable branch in the reusable workflow, can introduce bugs.
sorry, it was misleading to let it here as an example of lines of code required for the defaults (they match with the collection workflow used as example though), and it's possible to define ansible versions, as described in the reusable workflow:
sanity:
uses: data-bene/ansible-collection-gh-workflow/.github/workflows/workflow-sanity.yml@release/v1
with:
ansible-core-version: '[ "stable-2.12", "devel" ]'
I am in favor of pushing those action and workflow to one or the other ansible community organization, to not have external dependencies.
if you agree with the way I choose in the other PR for sanity test, I'll do something similar for those integration tests. I believe I addressed most if not all of the comments and feedback.
I suggest making things as simple as possible so that future contributors can quickly figure out how to adjust CI when needed. Also I suggest avoiding all unnecessary things (like licensing) and limiting the combinations as Felix mentioned. "Simplicity is a feature." and it would be nice not to wait for tests longer than 5-10 minutes max:)
@Andersson007 can you help me defined the ideal matrix for this collection regarding ansible core versions, docker images and PostgreSQL versions ?
For example:
- PostgreSQL all versions with ansible-2.12 and ansible-devel (or only 2.12 ?)
- PostgreSQL 14 with all ansible versions
- On a single unbuntu2004 or older ubuntu when 2004 is not available ?
@Andersson007 can you help me defined the ideal matrix for this collection regarding ansible core versions, docker images and PostgreSQL versions ?
@klando I think i should once again try to analyze the whole situation:) I'll do it this week. For now, you could move the file from data-bene's repo to this one. Thank you
I've just made another attempt and have found out that there's 4th repo involved. Frankly speaking, before creating the issue to move to GHA, i personally had expected to see the final solution looking not more complex than in c.mysql or c.dns. It's hard to read (for me personally), really.. The final solution should be based on a consensus among maintainers. I could vote for a solution if:
- it looks simple (like in c.dns / c.mysql)
- all stuff is in one place (in this repo)
- no encapsulating things (from other repos) - yes, far-from-ideal simplicity is much better than flexibility (to me).
I've just made another attempt and have found out that there's 4th repo involved. Frankly speaking, before creating the issue to move to GHA, i personally had expected to see the final solution looking not more complex than in c.mysql or c.dns. It's hard to read (for me personally), really.. The final solution should be based on a consensus among maintainers. I could vote for a solution if:
1. it looks simple (like in c.dns / c.mysql) 2. all stuff is in one place (in this repo) 3. no encapsulating things (from other repos) - yes, far-from-ideal simplicity is much better than flexibility (to me).
I have reduced in this way for the PR related to Sanity and asked for feedback on this other one before moving ahead on this one.