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

Jump to bottom

Test ansible 2.10 #448

Merged
merged 1 commit into from Oct 7, 2020
Merged

Test ansible 2.10 #448

merged 1 commit into from Oct 7, 2020

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Sep 23, 2020

[noissue]

@pulpbot
Copy link
Member

pulpbot commented Sep 23, 2020

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

@fao89 fao89 force-pushed the ansible210 branch 10 times, most recently from 5f08e5e to 8a60c26 Compare September 23, 2020 20:04
@fao89 fao89 mentioned this pull request Sep 23, 2020
Closed
@fao89 fao89 force-pushed the ansible210 branch 7 times, most recently from a9913bf to f90528a Compare September 23, 2020 21:34
@fao89 fao89 marked this pull request as ready for review September 23, 2020 21:35
mdellweg
mdellweg reviewed Sep 24, 2020
View reviewed changes
molecule/packages-dynamic/molecule.yml Outdated
@@ -1,7 +1,7 @@
---
dependency:
name: galaxy
role-file: requirements.yml
requirements-file: requirements.yml
Copy link
Member

Choose a reason for hiding this comment

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

given my observation, this will not work with the last version of molecule released for python2.

@fao89 fao89 force-pushed the ansible210 branch 2 times, most recently from c2c6f33 to 192a20e Compare September 24, 2020 20:31
mdellweg
mdellweg reviewed Sep 25, 2020
View reviewed changes
molecule/source-upgrade/molecule.yml Outdated
@@ -1,7 +1,7 @@
---
dependency:
name: galaxy
role-file: requirements.yml
requirements-file: requirements.yml
Copy link
Member

Choose a reason for hiding this comment

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

I believe those lines do not effect anything there can you try removing them?
In the docs they are found beneath options:
https://molecule.readthedocs.io/en/stable/configuration.html#ansible-galaxy

Copy link
Member Author

Choose a reason for hiding this comment

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

it is being used at dependency test: https://github.com/pulp/pulp_installer/runs/1162604142?check_suite_focus=true#step:9:28

Copy link
Member

Choose a reason for hiding this comment

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

the requirements.yml is read, because this is the default value of the dependency class in molecule.
Line 4 of this file is actually ignored, because it should have been:

options:
  requirements-file: requirements.yml

which will then be interpreted right in modern molecule, but call ansible-galaxy install -r requirements.yml --requirements-file requirements.yml on py2's molecule and rightfully fail with "I don't understand that parameter".
It's a mess and it took me some time to see what's going wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, requirements-file does not work, only works with roles-file which does not install the collection

Copy link
Member

Choose a reason for hiding this comment

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

And to add to the confusion, on ansible 2.10 is does install the collections in requirements.

@mdellweg
Copy link
Member

I don't know what i should say with this. It is (at least in parts) green, because it doesn't work properly.

It only "works", because for anything but ansible 2.10 the collection installation is not needed.

@fao89 fao89 force-pushed the ansible210 branch 3 times, most recently from 5dcfa96 to 2d5d879 Compare September 25, 2020 20:38
@fao89 fao89 marked this pull request as draft September 25, 2020 20:51
mdellweg
mdellweg reviewed Sep 28, 2020
View reviewed changes
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Left a few nit picky comments. What would it take to get this out of draft?

molecule/packages-dynamic/molecule.yml
@@ -1,7 +1,8 @@
---
dependency:
name: galaxy
role-file: requirements.yml
options:
role-file: requirements.yml
Copy link
Member

Choose a reason for hiding this comment

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

As that is molecules default, it should be ok to drop this altogether.

galaxy.yml Show resolved Hide resolved
molecule/packages-dynamic/molecule.yml
Comment on lines -46 to +48
- lint
- dependency
- lint
Copy link
Member

Choose a reason for hiding this comment

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

Is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes and no.
No - as we are installing the collection at tox.ini
Yes - if we were using the molecule to install the collection, lint would break because it wouldn't recognize the collection fqdn.
As I started the PR counting on molecule, I did this change, but now we don't need it anymore

Copy link
Member

Choose a reason for hiding this comment

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

"Yes and no" sounds like the theme of this PR...
Let's keep it that way, since a user can run this with molecule py3 and ansible 2.10 without the need to install collections by hand.

roles/pulp_devel/tasks/install_podman.yml Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@fao89
Copy link
Member Author

fao89 commented Sep 28, 2020

Left a few nit picky comments. What would it take to get this out of draft?

actually, it was in draft when I was studying molecule, and I forgot to move it back

@fao89 fao89 marked this pull request as ready for review September 28, 2020 14:40
mdellweg
mdellweg approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

I like this, and i am going to LGTM on it, but i'd like to see a second ACK on dropping ansible 2.8 tests, and therefore support.
Also we probably need a deprecation changelog here.

mikedep333
mikedep333 requested changes Sep 30, 2020
View reviewed changes
.yamllint Outdated Show resolved Hide resolved
galaxy.yml Show resolved Hide resolved
roles/pulp_devel/tasks/install_podman.yml Outdated Show resolved Hide resolved
roles/pulp_devel/tasks/install_podman.yml Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@fao89 fao89 requested a review from mikedep333 October 5, 2020 18:20
mikedep333
mikedep333 approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Thank you! I'm glad we're now moving forward to Ansible 2.10 at the best time.

@fao89 fao89 merged commit b4dc54b into pulp:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdellweg mdellweg mdellweg approved these changes

@mikedep333 mikedep333 mikedep333 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fao89 @pulpbot @mdellweg @mikedep333