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

AWS should only be sanity tested on python3 #915

Merged
merged 4 commits into from
May 25, 2021

Conversation

jillr
Copy link
Collaborator

@jillr jillr commented May 7, 2021

We're dropping python2 support in the next collection release, for both collections

@jillr
Copy link
Collaborator Author

jillr commented May 12, 2021

@pabelanger @GomathiselviS would either of you be able to review? And also advise - I'm not sure if the way I've gone about this is the best approach for the objective! (I'd prefer not to have to maintain hundreds of sanity ignores, especially for community.aws which has almost 200 modules)

@pabelanger
Copy link
Contributor

@pabelanger @GomathiselviS would either of you be able to review? And also advise - I'm not sure if the way I've gone about this is the best approach for the objective! (I'd prefer not to have to maintain hundreds of sanity ignores, especially for community.aws which has almost 200 modules)

you only want to test python3+ in the docker container right?

if so, we don't need to create new jobs for that. We can expose a new flag to ansible-test-sanity job to allow the user to pick the version of python to test against.

@jillr
Copy link
Collaborator Author

jillr commented May 12, 2021

@pabelanger Yep - the AWS collections will be py3.6+. So I'd edit the base ansible-test-sanity-docker (et al) jobs to take ansible_test_python: and then put that in zuul.d/ansible-cloud-jobs.yaml, with just that var specified?

@pabelanger
Copy link
Contributor

pabelanger commented May 12, 2021

So, i think what we need to do is update our ansible-test command to use

ansible-test sanity --requirements --docker -v --python=3.6,3.7,3.8

Do you mind confirming locally, that does what you are looking for?

@pabelanger
Copy link
Contributor

@jillr
Copy link
Collaborator Author

jillr commented May 12, 2021

So, i think what we need to do is update our ansible-test command to use

ansible-test sanity --requirements --docker -v --python=3.6,3.7,3.8

Do you mind confirming locally, that does what you are looking for?

ansible-test doesn't take a list for --python
ansible-test sanity: error: argument --python: invalid choice: '3.6,3.7,3.8' (choose from '2.6', '2.7', '3.5', '3.6', '3.7', '3.8', '3.9', '3.10', 'default')

@jillr
Copy link
Collaborator Author

jillr commented May 17, 2021

@pabelanger Should I submit some change to init_test_options.yaml? I'm not sure what our next step here is.

ansible_test_enable_ara: false
ansible_test_python: 3.6

- job:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since basically, all these jobs are 99% the same, I would inherit them from the first in the list and just overwrite the two different keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tbh I didn't entirely realize I could do that with parent! Let's see if I did that right or not. :)

@jillr jillr requested a review from goneri May 24, 2021 16:32
@jillr
Copy link
Collaborator Author

jillr commented May 24, 2021

I had to disabled the --docker option because https://github.com/ansible/ansible-zuul-jobs/blob/master/roles/ansible-test/tasks/init_test_options.yaml#L47-L52. I don't see any immediate issues with doing so (though my preference would be to isolate the tests, I'm disinclined to make changes to the base role).

@jillr
Copy link
Collaborator Author

jillr commented May 25, 2021

@pabelanger ready_for_review

@ansible-zuul ansible-zuul bot merged commit 14db656 into ansible:master May 25, 2021
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.

None yet

3 participants