spacelift-io / runner-ansible

MIT License
2 stars 6 forks source link

feat: Upgrade ansible to 10 #19

Closed jpadmin closed 2 months ago

jpadmin commented 3 months ago

Description of the change

This would upgrade ansible to 10 as there are bugs with the existing system. A popular usecase : https://github.com/ansible/ansible/pull/80751 A very low level demonstration of the bug

$ ansible-galaxy collection install community.docker
Starting galaxy collection install process
Process install dependency map
[WARNING]: Skipping Galaxy server https://galaxy.ansible.com. Got an unexpected error when getting available versions of collection community.docker: Unknown error when
attempting to call Galaxy at 'https://galaxy.ansible.com/api/': HTTPSConnection.__init__() got an unexpected keyword argument 'cert_file'. HTTPSConnection.__init__() got an
unexpected keyword argument 'cert_file'
ERROR! Unknown error when attempting to call Galaxy at 'https://galaxy.ansible.com/api/': HTTPSConnection.__init__() got an unexpected keyword argument 'cert_file'. HTTPSConnection.__init__() got an unexpected keyword argument 'cert_file'

There are two ways to fix, either to switch to python 3.11 or upgrade ansible and I would prefer to upgrade ansible.

Type of change

Checklists

Development

Code review

eliecharra commented 3 months ago

I don't think we can introduce this change as it. It's gonna introduce BC breaks for everyone using this image. Pinning it to ansible v7 was on purpose, to avoid introducing unwanted BC breaks.

We probably need to change the way we tag this docker image to be able to introduce a new version of Ansible without breaking existing flows.

WDYT @peterdeme ?

@michalg9 Do you have any plans on your side to refactor a bit how we tag our runner images as part of the Ansible improvement work? I also use Ansible on one of my personal stacks, and the image Spacelift provides is just broken. This makes Spacelift default broken which is a bit sad. I would really like us to maintain a multi-tag image to allow people to choose their major version of Ansible. Something like public.ecr.aws/spacelift/runner-ansible:10

@jpadmin For now, as a workaround, I would recommend building your own image. Here is below a minimal Dockerfile that you can use:

FROM python:3.12
ARG ANSIBLE_VERSION=10.0
RUN pip install ansible~=${ANSIBLE_VERSION} ansible-runner~=2.4
peterdeme commented 3 months ago

Agreed with Elie, proper tagging would be cool:

Perhaps let's get rid of latest tag and have our customers explicitly state which one they want? opinion?

eliecharra commented 3 months ago

Perhaps let's get rid of latest tag and have our customers explicitly state which one they want? opinion?

I do not have a strong opinion, other than not having a :latest can be a pain if you just want to use a tool without caring about the version. I don't think it can hurt to support latest

peterdeme commented 3 months ago

@eliecharra fair, but currently a latest (eg. ansible 10) tag would be a breaking change

eliecharra commented 3 months ago

@eliecharra fair, but currently a latest (eg. ansible 10) tag would be a breaking change

Yeah totally.

My vision of it is that we should just archive this repo and create a new one with clean versioning of Ansible versions. That should be fairly easy to do with a build matrix and docker build args. A bit like in the Dockerfile example I pasted above. We need to publish that under a brand new ECR repo.

jpadmin commented 3 months ago

A bit like in the Dockerfile example I pasted above

I suggest using python:3.12-alpine as the base image and adding ssh-cli on top. These features are what I find particularly helpful when comparing your images with other runner images.

eliecharra commented 3 months ago

Let's see if @michalg9 has any thoughts on that

peterdeme commented 3 months ago

My vision of it is that we should just archive this repo and create a new one with clean versioning of Ansible versions.

That'd could work too, though I'd be a bit sad because I like the runner-<tooling> naming convention that we nicely followed so far. :D

eliecharra commented 2 months ago

Closing in favor of #21

@jpadmin Can you try it with one of the following images, and let us know if it works for you?

public.ecr.aws/spacelift/runner-ansible:10
public.ecr.aws/spacelift/runner-ansible:10-gcp
public.ecr.aws/spacelift/runner-ansible:10-aws
public.ecr.aws/spacelift/runner-ansible:10-azure

You can also use a tag pointing to a minor version if you want better control

public.ecr.aws/spacelift/runner-ansible:10.1
public.ecr.aws/spacelift/runner-ansible:10.1-gcp
public.ecr.aws/spacelift/runner-ansible:10.1-aws
public.ecr.aws/spacelift/runner-ansible:10.1-azure

Also, bear in mind that this is still a work in progress, so please do not use this image for production yet πŸ™πŸ»

jpadmin commented 2 months ago

@eliecharra ,

thanks for the changes, though I find some issues with user permissions but with root it works.

docker run -it public.ecr.aws/spacelift/runner-ansible:10 sh         
~ $ ansible-galaxy collection install community.docker
ERROR: Unable to create local directories(/.ansible/tmp): [Errno 13] Permission denied: b'/.ansible'
~ $ whoami
whoami: unknown uid 1983
eliecharra commented 2 months ago

@jpadmin Haha you are too fast for the CI pipeline. I fixed this issue already, but the build is still in progress.

Can you retry once this pipeline is done ? πŸ™πŸ»

jpadmin commented 2 months ago

sure

jpadmin commented 2 months ago

works now, thanks a lot πŸ‘

eliecharra commented 2 months ago

Amazing, thanks a lot for your feedback πŸ™πŸ» When PR #21 gets merged, it will be safe for prod purposes.