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

Fix lint errors and update molecule for ansible v2.8 #23

Conversation

lhoss
Copy link

@lhoss lhoss commented Jun 25, 2019

Changes proposed in this pull request:

  • Fixes all yaml lint and ansible lint errors that appeared due to recent ansible-lint module updates
  • also had to update to the latest molecule version, for compatibility with ansible v2.8.x (which got used automatically by using recent docker containers, at least in my kdc-with-replica tests, see upcoming PR)
  • The explicit install of cerberus v1.3.1, indeed required (to fix some schema error), can be removed once we have a molecule release that contains the fix from Bump cerberus ansible/molecule#2103 (merged to master since the 17.06.2019)

Full credits go to @NadOby, who initially contributed these fixes to a PR in our repo (
scigility#1), which I had to rebase, because it was derived from an unfinished branch

@NadOby
Copy link

NadOby commented Jul 2, 2019

This one is pretty straightforward, the only fishy part is fixing versions of packages in .travis.yml
This should be revisited in a month or two, hopefully, molecule will update from master when.

@lhoss
Copy link
Author

lhoss commented Jul 2, 2019

@HorizonNet , @matb Any interest in merging this? (and see also #22)

@HorizonNet
Copy link
Member

@lhoss I've followed the discussions over the last few days, but didn't have the time yet to look into the changes. One of our colleagues is currently checking and testing the changes. I hope that I find some time over the weekend to look into them.

@murtazahassan123
Copy link

@lhoss
Just a small question; why are you using release candiadate and not release version in "pip install molecule==2.22rc1"?

Copy link
Member

@HorizonNet HorizonNet left a comment

Choose a reason for hiding this comment

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

LGTM overall. Left a minor comment.

@HorizonNet
Copy link
Member

I also like that you're going to improve the quality score of the module. Thanks!

@lhoss
Copy link
Author

lhoss commented Jul 15, 2019

Just a small question; why are you using release candiadate and not release version in "pip install molecule==2.22rc1"?

@SyedMurtazaHassan I had to use that rc1 version because of the 2nd point I mentioned (in PR desc.):

  • also had to update to the latest molecule version, for compatibility with ansible v2.8.x (which got used automatically by using recent docker containers, at least in my kdc-with-replica tests, see upcoming PR)

Please use a stable version of molecule instead of a release candidate.

@HorizonNet I just checked if there's a newer release out, but nope!

So if we want to merge this these days (not waiting for new molecule release), I do not see another option than:

  • Downgrade molecule again to latest stable and thus fixing the ansible version in the build to a recent 2.7.x (and hoping the other done test improvements still work with older molecule /testinfra deps)
  • or stay with the molecule 'rc' version (I just tested with the latest RC 2.22rc3, which worked well, plus this version fixed the cerberus+flake8 pip deps)

Pragmatically, to not loose time (getting it working) with the old molecule version (that seems also much slower during infratests taking 80s vs 10s on my Macbook), I'ld favor to accept the 'rc' version, since it's working nicely (and of course changing to the release as soon as it is released)
Wdyt ?

@lhoss
Copy link
Author

lhoss commented Aug 26, 2019

Please use a stable version of molecule instead of a release candidate.

@HorizonNet I just checked if there's a newer release out, but nope!

Finally a new molecule stable version came out (2.22), the 18.8.2019!
I just updated the PR to that version, so it can be finally merged, @HorizonNet !
Will rebase this change also in the other open PR!

Copy link
Member

@HorizonNet HorizonNet left a comment

Choose a reason for hiding this comment

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

LGTM. Let's wait for the build results.

@HorizonNet HorizonNet merged commit 64ec97e into ultratendency:master Dec 14, 2019
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.

4 participants