search

projectatomic / atomic-host-tests

A collection of single-host tests for Atomic Host
GNU General Public License v3.0
18 stars 21 forks source link

tree: changes needed to support Ansible 2.4 #373

Closed miabbott closed 6 years ago

miabbott commented 6 years ago

The main intention of these changes is to get the tests to a point where they can be used with Ansible 2.4 without any errors/warnings. In order to get there, I had to update some of the roles along the way.

Specifically, I updated the atomic install/uninstall related roles to use unique variables names to prevent collisions during tests. I also updated the rpm_version role to be more specific where tasks were being included from and how variables were being passed to include_tasks.

With those changes in place, I was able to update the tests to use include_tasks where a bare include was used.

miabbott commented 6 years ago

This is a follow-on to #372, which needs to be reviewed and merged first. I'll rebase after that PR gets in and this can be reviewed then.

As such, this also addresses #336

miabbott commented 6 years ago

Ah, this will be fun...chicken and egg problem where include_tasks isn't supported until Ansible 2.4. But the PAPR CI uses Ansible 2.2.

I guess I could just update requirements.txt to use 2.4 as part of this PR...

mike-nguyen commented 6 years ago

I meant to leave a comment on #372 about include_task. I'm not sure how merging these separate PRs might impact the tests with backwards compatibility.

miabbott commented 6 years ago

CI is a mess because of broken versions of 'atomic' and unsupported tests on F27. I think the playbooks themselves should be sound; they at least pass a syntax check.

I'll be running all the changed playbooks against some hosts to verify nothing has broken.

miabbott commented 6 years ago

I've run all the supported tests against a F27 host without errors.

I'll continue to run against other platforms, but I think it's in a place where it could be fully reviewed/tested by others.

mike-nguyen commented 6 years ago

@miabbott can you resolve the tests/rpm-ostree/main.yml conflict? I think it will re-run the CI #379 merged

miabbott commented 6 years ago

@mike-nguyen I rebased on to upstream/master, squashed the fixups, and force pushed. I think for this set of commits, we should do a 'rebase and merge' rather than 'squash and merge' - it should make tracking the changes a bit easier, IMO.

mike-nguyen commented 6 years ago

@miabbott Rebase and merge sounds good based on the amount of changes in this PR. I'll take a look when the CI finishes up.

miabbott commented 6 years ago

Squashed a fixup into the existing commits and force pushed them all again. :arrow_up:

mike-nguyen commented 6 years ago

bot, retest this please

mike-nguyen commented 6 years ago

The failures aren't all consistent. I'm going to look at this locally to see what's up.

mike-nguyen commented 6 years ago

I ran the PR against F27 with all the touched tests and they passed. @miabbott Let me know how the centos 7 results look and I can merge if no issues are found.

miabbott commented 6 years ago

@mike-nguyen Sorry it took me so long to report back (doing a lot of context switching today).

I ran the tests in order on the same CentOS AH 7 VM like the PAPR does and encountered similar failures, but they aren't related to the the changes themselves. I think the some of the tests are leaving a "dirty" state on the VM and it is tripping up the subsequent tests.

Having said all that, I'm comfortable with merging this if you are.

mike-nguyen commented 6 years ago

@miabbott I ran each test on a fresh centos 7 vm last night to see if I could get a clean run but ran into a lot of failures. I'm going to take a look and if they aren't related to these changes, I'm going to merge.

mike-nguyen commented 6 years ago

I'm just going to merge this as the failures don't look related to the changes. We can deal with the fallout if any occurs.