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

Ci #1218

Merged
merged 6 commits into from
May 6, 2024
Merged

Ci #1218

merged 6 commits into from
May 6, 2024

Conversation

eb4x
Copy link
Collaborator

@eb4x eb4x commented May 3, 2024

SUMMARY

This attempts to fix some shortcomings in the CI

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

molecule, github-actions

ADDITIONAL INFORMATION

It would be good if this could get merged, the change to molecule/requirements.txt results in a lot of unnecessary testing of other roles during my refactoring of the code.

@eb4x
Copy link
Collaborator Author

eb4x commented May 5, 2024

@pyrodie18

Any idea or guesses as to why the molecule verify step fails? It spits out Could not resolve host: zabbix-web-rockylinux9, but I don't see how it was able to resolve before before the namechange of the containers?

If I docker exec -it zabbix-web-rockylinux9 /bin/bash into the container and ping zabbix-web-rockylinux9, it resolves. Is the verify step happening on the controller, and do we supply some /etc/hosts overrides any other thing that needs an update?

molecule==4.0.4
molecule-docker==2.1.0
molecule<5
molecule-docker @ git+https://github.com/ansible-community/molecule-docker@main
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to change the source to straight to github?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, there's a fix that went in after 2.1.0, cb1c1c31a4a468fb243fc2000656504a3ca00bb3
that fixes a bug where the molecule.yml would get overwritten during image building. (I introduced the simple image-building to get python3 on centos7. But I think it's a good way for us to always stay current on all distribution images, instead of the geerlingguy images)

Copy link
Collaborator Author

@eb4x eb4x May 5, 2024

Choose a reason for hiding this comment

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

Also, the repo is deprecated/archived, so there won't come any further changes to it, but I could pin it to the hash instead?

We should probably go over to molecule 5, but I'd rather get these refactors in before overhauling the new molecule version.

@@ -68,11 +68,19 @@ jobs:
- name: Build the collection
run: |
# Pin versions to speed up CI
sed -i 's/ansible\.windows:\s*"\*"/ansible.windows: "2.3.0"/' galaxy.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do this? In my mind we should be testing against what we publish in the galaxy.yml file otherwise we may have an issue of experiencing/not experiencing failures that we should/shouldn't during CI/CD testing. Not to mention having to change versions in 5 places instead one 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part where galaxy looks up every single version, for every single dependency where we use * takes multiple minutes, multiply that by the matrix of configurations we're testing in github workflows, you're looking at a couple hours just to resolve these dependencies. (Yes, it's really that stupid, I've provided links to reported issues in ce6b2da)

I think it's better we pin this for CI only, as a user could have installed a different collection that pins postgresql to X.Y.Z and if we're fine with that (given we set it to *), galaxy will say requirements are already met and leave it alone anyway, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pinning in CI is not ideal, and especially since it's in this many places, but pinning in galaxy.yml would be the worse of two options.

In any case we should probably specify minimum requirements for all the * dependencies though.

@eb4x
Copy link
Collaborator Author

eb4x commented May 5, 2024

Any idea or guesses as to why the molecule verify step fails? It spits out Could not resolve host: zabbix-web-rockylinux9, but I don't see how it was able to resolve before before the namechange of the containers?

Nevermind, I think I see the error of my ways. It's not the full name as used above.

eb4x added 5 commits May 5, 2024 20:21
The self-hosted GitHub actions-runner does not scale with a single
runner on a "powerful" machine.

But we can create multiple users on a single machine, and have each
user run an actions-runner. Problem is they all share the same
docker daemon. And when multiple runners try starting up a container
with the same name as an existing one, the jobs fail.

This is an attempt to make the container names unique enough for
each job, and allows us to scale out actions-runners on a single
machine.
Following up on unique names for containers, we need unique names
for their corresponding database-container during testing.
Grab the latest packaged molecule 4 release.

And the latest molecule-docker straight from git. There's a fix
that went in after 2.1.0 (cb1c1c31a4a468fb243fc2000656504a3ca00bb3)
that fixes a bug where the molecule.yml would get overwritten
during image building.
We don't want to update galaxy.yml directly where others might need
different versions because of requirements, but pinning it here to
speed up the CI seems like a good compromise.
@pyrodie18 pyrodie18 merged commit e00ab8e into ansible-collections:main May 6, 2024
280 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants