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.

Sign up for GitHub

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

Add cloudscale.ch API inventory plugin #53517

Merged
merged 1 commit into from
Apr 10, 2019
Merged

Add cloudscale.ch API inventory plugin #53517

merged 1 commit into from
Apr 10, 2019

Conversation

gaudenz
Copy link
Contributor

@gaudenz gaudenz commented Mar 8, 2019

SUMMARY

cloudscale.ch API inventory plugin

ISSUE TYPE
  • New Module Pull Request
    (actually new plugin)
COMPONENT NAME

cloudscale

ADDITIONAL INFORMATION

Inventory plugin as a companion to the already existing cloudscale.ch cloud modules.

@ansibot
Copy link
Contributor

ansibot commented Mar 8, 2019

cc @resmo
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud community_review In order to be merged, this PR must follow the community review workflow. needs_triage Needs a first human triage before being processed. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Mar 8, 2019
@gaudenz gaudenz force-pushed the cloudscale_inventory_plugin branch from 97dcab5 to b04c60e Compare March 11, 2019 13:45
resmo
resmo reviewed Mar 15, 2019
View reviewed changes
Copy link
Contributor

@resmo resmo left a comment

Choose a reason for hiding this comment

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

good job. only minor doc issues. ready for merge i would say

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 15, 2019
@gaudenz gaudenz force-pushed the cloudscale_inventory_plugin branch from b04c60e to 2f075c2 Compare March 18, 2019 16:19
@gaudenz
Copy link
Contributor Author

gaudenz commented Mar 18, 2019

ready_for_review

@gaudenz gaudenz force-pushed the cloudscale_inventory_plugin branch from 2f075c2 to bfe1090 Compare March 18, 2019 22:57
@gaudenz
Copy link
Contributor Author

gaudenz commented Mar 18, 2019

ready_for_review

resmo
resmo approved these changes Mar 18, 2019
View reviewed changes
Copy link
Contributor

@resmo resmo left a comment

Choose a reason for hiding this comment

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

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 18, 2019
bcoca
bcoca requested changes Mar 18, 2019
View reviewed changes
lib/ansible/plugins/inventory/cloudscale.py Outdated
# Two servers with the same name exist, create a group
# with this name and add the servers by UUID
if name not in self.inventory.groups:
self.inventory.add_group(name)
Copy link
Member

Choose a reason for hiding this comment

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

add_group creates and returns a 'sanitized' group name, to avoid warnings you might want to use self._sanitize_group_name(name)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I thought this would already made by add_group().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that although I really don't like the fact that this will convert dashes (-) to underscores (_). But this is not specific to this module and if Ansible is really going this way we should probably not allow legacy group names on a new inventory plugin.

Copy link
Member

Choose a reason for hiding this comment

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

@resmo it is, but they are not using the results to compare, so same group will never be in inventory.groups (which you don't need to do since add_group already checks this).

Copy link
Member

Choose a reason for hiding this comment

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

@gaudenz for those that really want it, there will be global toggle available, you CAN add toggle at plugin level to do same

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed shipit This PR is ready to be merged by Core labels Mar 18, 2019
@resmo
Copy link
Contributor

resmo commented Mar 19, 2019

also the verify_file from the super class BaseInventoryPlugin should be implemented.

@resmo resmo self-requested a review March 19, 2019 06:30
@gaudenz
Copy link
Contributor Author

gaudenz commented Mar 19, 2019

What should the verify_file method check? I looked at similar plugins and what they seem to do is check that the file has a specific ending (like aws_ec2.yml). This looks like a silly restriction to me as either the user has enabled the plugin explicitly in his ansible configuration or if it's loaded by the auto plugin there is a line plugin: cloudscale in the file. In either case it's quite obvious that this file is meant to be used by the cloudscale plugin and I don't see the point of adding further restrictions.

There are currently no required parameters which don't have a default value which we could check.

@bcoca
Copy link
Member

bcoca commented Mar 19, 2019 •
edited

its there because the user might have N plugins enabled and this allows for a very quick filter (avoiding reading the file itself) to skip/handle the source. Yes the auto plugin can take care of it, but it adds a layer of indirection and always requires reading the file.

Also its an easy way to avoid 'extra' errors since when we don't have a good filter on verify_file we are attempting to use multiple plugins and if they all fail, we have to return ALL errors, since we are unsure which one the user meant to use.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 27, 2019
@gaudenz gaudenz force-pushed the cloudscale_inventory_plugin branch from bfe1090 to 35c134a Compare April 9, 2019 13:51
@gaudenz
Copy link
Contributor Author

gaudenz commented Apr 9, 2019

ready_for_review

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 9, 2019
resmo
resmo approved these changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@resmo resmo left a comment

Choose a reason for hiding this comment

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

shipit

@resmo resmo merged commit a290cb4 into ansible:devel Apr 10, 2019
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bcoca bcoca bcoca requested changes

@resmo resmo resmo approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud community_review In order to be merged, this PR must follow the community review workflow. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gaudenz @ansibot @resmo @bcoca