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

feat: Upgrade ansible to 10 #19

Closed
wants to merge 2 commits into from
Closed

Conversation

jpadmin
Copy link

@jpadmin jpadmin commented Jul 19, 2024

Description of the change

This would upgrade ansible to 10 as there are bugs with the existing system.
A popular usecase : ansible/ansible#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

  • Bug fix (non-breaking change that fixes an issue);
  • New feature (non-breaking change that adds functionality);
  • Breaking change (fix or feature that would cause existing functionality not to work as expected);
  • Other (miscellaneous, GitHub workflow changes, changes to the PR template);

Checklists

Development

  • Parts of this pull request impacting core (user-facing, documented) product functionality have test coverage;

Code review

  • This pull request has a descriptive title and sufficient context for a reviewer. There may be a screenshot or screencast attached;
  • This pull request is no longer a draft;
  • You have reviewed this Pull Request yourself;

@jpadmin jpadmin requested a review from a team as a code owner July 19, 2024 19:35
@eliecharra
Copy link
Member

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
Copy link
Contributor

Agreed with Elie, proper tagging would be cool:

  • ansible:7
  • ansible:10
  • and ansible:latest which points to the latest one, although that's pretty risky because could bump a major version overnight.

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

@eliecharra
Copy link
Member

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
Copy link
Contributor

peterdeme commented Jul 22, 2024

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

@eliecharra
Copy link
Member

@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
Copy link
Author

jpadmin commented Jul 22, 2024

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
Copy link
Member

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

@peterdeme
Copy link
Contributor

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 eliecharra mentioned this pull request Jul 24, 2024
8 tasks
@eliecharra
Copy link
Member

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 🙏🏻

@eliecharra eliecharra closed this Aug 8, 2024
@jpadmin
Copy link
Author

jpadmin commented Aug 8, 2024

@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
Copy link
Member

@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
Copy link
Author

jpadmin commented Aug 8, 2024

sure

@jpadmin
Copy link
Author

jpadmin commented Aug 8, 2024

works now, thanks a lot 👍

@eliecharra
Copy link
Member

Amazing, thanks a lot for your feedback 🙏🏻
When PR #21 gets merged, it will be safe for prod purposes.

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.

3 participants