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

Improvements on "ansiblegate" module #60056

Merged
merged 33 commits into from Jul 12, 2022

Conversation

meaksh
Copy link
Contributor

@meaksh meaksh commented Apr 20, 2021

What does this PR do?

This PR adds some improvements to the "Ansiblegate" module of Salt:

  • Move Ansible inventory collector from "AnsibleRoster" to Salt utils to reuse it
  • Add ansible.targets method to gather Ansible inventory
  • Add ansible.discover_playbooks method to help collecting playbooks.
  • Fix crash when running Ansible playbooks if ansible-playbook CLI output is not the expected JSON.
  • Fix issues when processing inventory and there are groups with no members.
  • Allow new types of targets for Ansible roster

Merge requirements satisfied?

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@meaksh meaksh requested a review from a team as a code owner April 20, 2021 11:55
@meaksh meaksh requested review from twangboy and removed request for a team April 20, 2021 11:55
@meaksh meaksh force-pushed the master-ansiblegate-improvements branch from 44295aa to 4f7c596 Compare April 20, 2021 15:01
@meaksh meaksh force-pushed the master-ansiblegate-improvements branch from 0f8ee2c to eb0b3ae Compare September 23, 2021 15:51
@meaksh
Copy link
Contributor Author

meaksh commented Oct 1, 2021

@twangboy @Ch3LL any chance of reviewing this one? Thanks!

@meaksh
Copy link
Contributor Author

meaksh commented Feb 7, 2022

@s0undt3ch maybe you can take a look at this one? Thanks in advance!

BTW, just wondering what is the propose of using [:] to get a copy of the declarated list at
https://github.com/saltstack/salt/pull/60056/files#diff-519146c38c0ed7bc961ffc5f408d3dd380abb8760d48e839743dbd38dc28ba15L47

@s0undt3ch
Copy link
Member

@s0undt3ch maybe you can take a look at this one? Thanks in advance!

BTW, just wondering what is the propose of using [:] to get a copy of the declarated list at https://github.com/saltstack/salt/pull/60056/files#diff-519146c38c0ed7bc961ffc5f408d3dd380abb8760d48e839743dbd38dc28ba15L47

It's a failed implementation at separate copies of the same original list....

>>> a = b = [1, 2, 3]
>>> a
[1, 2, 3]
>>> b
[1, 2, 3]
>>> a.remove(2)
>>> a
[1, 3]
>>> b
[1, 3]
>>> a = b = [1, 2, 3][:]
>>> a
[1, 2, 3]
>>> b
[1, 2, 3]
>>> a.remove(2)
>>> a
[1, 3]
>>> b
[1, 3]
>>> a = b[:] = [1, 2, 3][:]
>>> a
[1, 2, 3]
>>> b
[1, 2, 3]
>>> a.remove(2)
>>> a
[1, 3]
>>> b
[1, 2, 3]

ie, a bug.

😭

@s0undt3ch
Copy link
Member

The correct code should

@s0undt3ch maybe you can take a look at this one? Thanks in advance!
BTW, just wondering what is the propose of using [:] to get a copy of the declarated list at https://github.com/saltstack/salt/pull/60056/files#diff-519146c38c0ed7bc961ffc5f408d3dd380abb8760d48e839743dbd38dc28ba15L47

It's a failed implementation at separate copies of the same original list....

>>> a = b = [1, 2, 3]
>>> a
[1, 2, 3]
>>> b
[1, 2, 3]
>>> a.remove(2)
>>> a
[1, 3]
>>> b
[1, 3]
>>> a = b = [1, 2, 3][:]
>>> a
[1, 2, 3]
>>> b
[1, 2, 3]
>>> a.remove(2)
>>> a
[1, 3]
>>> b
[1, 3]
>>> a = b[:] = [1, 2, 3][:]
>>> a
[1, 2, 3]
>>> b
[1, 2, 3]
>>> a.remove(2)
>>> a
[1, 3]
>>> b
[1, 2, 3]

ie, a bug.

sob

The correct code should be:

>>> a = b[:] = [1, 2, 3]
>>> a
[1, 2, 3]
>>> b
[1, 2, 3]
>>> a.remove(2)
>>> a
[1, 3]
>>> b
[1, 2, 3]
>>>

Copy link
Member

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

Here's an initial review

salt/modules/ansiblegate.py Outdated Show resolved Hide resolved
salt/modules/ansiblegate.py Outdated Show resolved Hide resolved
salt/utils/ansible.py Outdated Show resolved Hide resolved
@meaksh
Copy link
Contributor Author

meaksh commented Feb 7, 2022

@s0undt3ch thanks for the review! 👍

I've pushed the changes you suggested on the review

@garethgreenaway
Copy link
Contributor

@meaksh Looks like this one has some merge conflicts.

@meaksh
Copy link
Contributor Author

meaksh commented Apr 22, 2022

Thanks for the review @garethgreenaway !

I've have rebased with latest master so conflicts are now solved.

twangboy
twangboy previously approved these changes May 2, 2022
@Ch3LL
Copy link
Contributor

Ch3LL commented Jun 27, 2022

there is a merge conflict that needs to be resolved.

@meaksh
Copy link
Contributor Author

meaksh commented Jun 28, 2022

@Ch3LL conflicts are now fixed 👍

salt/modules/ansiblegate.py Show resolved Hide resolved
salt/modules/ansiblegate.py Outdated Show resolved Hide resolved
salt/modules/ansiblegate.py Show resolved Hide resolved
salt/modules/ansiblegate.py Show resolved Hide resolved
@meaksh
Copy link
Contributor Author

meaksh commented Jun 29, 2022

@Ch3LL thanks again for the review. I've addressed your comments and I think this is now ready.

I hope it can gets into 3005!

Ch3LL
Ch3LL previously approved these changes Jun 29, 2022
s0undt3ch
s0undt3ch previously approved these changes Jun 29, 2022
@Ch3LL
Copy link
Contributor

Ch3LL commented Jul 7, 2022

Unfortunately I was not able to get this into 3005 since we were on a tight timeline. I'll go ahead and update the versionadded versions to 3006 and help get this merged in. I'll also label it 3006 so it for sure makes it into that release.

@Ch3LL Ch3LL added the Sulfur v3006.0 release code name and version label Jul 7, 2022
@Ch3LL Ch3LL dismissed stale reviews from s0undt3ch and themself via 6d4d351 July 7, 2022 18:50
@Ch3LL Ch3LL merged commit d674063 into saltstack:master Jul 12, 2022
@Ch3LL
Copy link
Contributor

Ch3LL commented Jul 14, 2022

So we actually ended up rebasing master with the release, so this will actually end up in 3005 ! yay! I also updated the versionadded to 3005: #62331

@meaksh meaksh deleted the master-ansiblegate-improvements branch July 15, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants