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

Added OpenNebula inventory plugin #810

Merged

Conversation

feldsam
Copy link
Contributor

@feldsam feldsam commented Aug 20, 2020

SUMMARY

Added new inventory plugin for use with OpenNebula based clouds

ISSUE TYPE
  • New Plugin Pull Request
COMPONENT NAME

opennebula inventory plugin

REFS ansible/ansible#66394

@ansibullbot ansibullbot added affects_2.10 community_review inventory inventory plugin needs_triage new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) labels Aug 20, 2020
@feldsam feldsam force-pushed the inventory-opennebula branch 2 times, most recently from c3298be to 6adaf5b Compare August 20, 2020 21:41
@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI stale_ci CI is older than 7 days, rerun before merging labels Oct 21, 2020
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@feldsam thanks for the PR!
In additional to added, all new stuff should have ci or unit tests

plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Show resolved Hide resolved
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI stale_ci CI is older than 7 days, rerun before merging labels Oct 21, 2020
@Andersson007
Copy link
Contributor

plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
@Andersson007
Copy link
Contributor

looks like unit tests (with pytest) is all that we need

@feldsam
Copy link
Contributor Author

feldsam commented Oct 28, 2020

Hello, I never do it, would you point me to some example? Thanks

@Andersson007
Copy link
Contributor

Andersson007 commented Oct 28, 2020

Hi, sure, there are a lot of examples living in tests/units/ subdirectories including tests for plugins
The aim is to cover your code as full as possible (maybe split it to more functions would be required). It'll help be sure the covered things work as expected and will protect the plugin if anybody accidentally breaks something when fixing a bug or adding features.
Some old of them use unittest but Ansible documentation says to use pytest. If any questions, feel free to ask.

@felixfontein
Copy link
Collaborator

As concrete examples for inventory plugins, see https://github.com/ansible-collections/community.general/blob/main/tests/unit/plugins/inventory/test_proxmox.py and https://github.com/ansible-collections/community.general/blob/main/tests/unit/plugins/inventory/test_stackpath_compute.py. These were added recently (and you can see that one of them was inspired by the other :) ).

@feldsam
Copy link
Contributor Author

feldsam commented Oct 28, 2020

Thank you guys, I look into it in one-two weeks.

@felixfontein
Copy link
Collaborator

@feldsam are you still working on this?

needs_info

@ansibullbot ansibullbot added needs_info This issue requires further information. Please answer any outstanding questions stale_ci CI is older than 7 days, rerun before merging and removed community_review labels Feb 26, 2021
@feldsam
Copy link
Contributor Author

feldsam commented Mar 3, 2021

@felixfontein Hi, I am just asked community to help write tests for this plugin.

@ansibullbot ansibullbot added community_review and removed needs_info This issue requires further information. Please answer any outstanding questions labels Mar 3, 2021
Signed-off-by: Kristian Feldsam <feldsam@gmail.com>
Signed-off-by: Kristian Feldsam <feldsam@gmail.com>
@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 27, 2021
Signed-off-by: Kristian Feldsam <feldsam@gmail.com>
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 27, 2021
@feldsam
Copy link
Contributor Author

feldsam commented Sep 27, 2021

Hi @felixfontein I just added unit tests

@ansibullbot ansibullbot removed the needs_info This issue requires further information. Please answer any outstanding questions label Sep 27, 2021
@felixfontein felixfontein removed the waiting_on_contributor Needs help. Feel free to engage to get things unblocked label Sep 28, 2021
@ansibullbot ansibullbot added the waiting_on_contributor Needs help. Feel free to engage to get things unblocked label Sep 28, 2021
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Just two things that need to be updated, I think then this is ready for merging!

plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
plugins/inventory/opennebula.py Outdated Show resolved Hide resolved
@felixfontein felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Sep 28, 2021
Co-authored-by: Felix Fontein <felix@fontein.de>
@feldsam
Copy link
Contributor Author

feldsam commented Sep 28, 2021

@felixfontein done

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @feldsam !!

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Sep 28, 2021
@felixfontein felixfontein merged commit 806f1ea into ansible-collections:main Sep 28, 2021
@patchback
Copy link

patchback bot commented Sep 28, 2021

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/806f1ea3c99a5b77791d0a5afc5680f69549e001/pr-810

Backported as #3460

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Sep 28, 2021
patchback bot pushed a commit that referenced this pull request Sep 28, 2021
* Added OpenNebula inventory plugin

Signed-off-by: Kristián Feldsam <feldsam@gmail.com>

* Apply suggestions from code review

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

* Removed matching inventory yaml files ending with "one"

Too general word

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

* Apply suggestions from code review

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Added BOTMETA

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Moved import

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Fix indentation problem

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Added group_by_labels, refactored so can be unit tested

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Added unit tests

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Removed blank line

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
(cherry picked from commit 806f1ea)
@felixfontein
Copy link
Collaborator

@feldsam thanks for contributing this!
@Andersson007 @russoz thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Sep 28, 2021
* Added OpenNebula inventory plugin

Signed-off-by: Kristián Feldsam <feldsam@gmail.com>

* Apply suggestions from code review

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

* Removed matching inventory yaml files ending with "one"

Too general word

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

* Apply suggestions from code review

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Added BOTMETA

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Moved import

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Fix indentation problem

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Added group_by_labels, refactored so can be unit tested

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Added unit tests

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Removed blank line

Signed-off-by: Kristian Feldsam <feldsam@gmail.com>

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
(cherry picked from commit 806f1ea)

Co-authored-by: Kristian Feldsam <feldsam@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inventory inventory plugin new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit waiting_on_contributor Needs help. Feel free to engage to get things unblocked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants