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

Add cloudscale.ch API inventory plugin #53517

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@gaudenz
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 8, 2019

@resmo
Copy link
Member

resmo left a comment

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

@ansibot ansibot removed the needs_triage label Mar 15, 2019

@gaudenz gaudenz force-pushed the cloudscale-ch:cloudscale_inventory_plugin branch from b04c60e to 2f075c2 Mar 18, 2019

@gaudenz

This comment has been minimized.

Copy link
Contributor Author

gaudenz commented Mar 18, 2019

ready_for_review

@gaudenz gaudenz force-pushed the cloudscale-ch:cloudscale_inventory_plugin branch from 2f075c2 to bfe1090 Mar 18, 2019

@gaudenz

This comment has been minimized.

Copy link
Contributor Author

gaudenz commented Mar 18, 2019

ready_for_review

@resmo

resmo approved these changes Mar 18, 2019

Copy link
Member

resmo left a comment

shipit

@ansibot ansibot added shipit and removed community_review labels Mar 18, 2019

# 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)

This comment has been minimized.

@bcoca

bcoca Mar 18, 2019

Member

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

This comment has been minimized.

@resmo

resmo Mar 19, 2019

Member

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

This comment has been minimized.

@gaudenz

gaudenz Mar 19, 2019

Author Contributor

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.

This comment has been minimized.

@bcoca

bcoca Mar 19, 2019

Member

@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).

This comment has been minimized.

@bcoca

bcoca Mar 19, 2019

Member

@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 and removed shipit labels Mar 18, 2019

@resmo

This comment has been minimized.

Copy link
Member

resmo commented Mar 19, 2019

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

@resmo resmo self-requested a review Mar 19, 2019

@gaudenz

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

bcoca commented Mar 19, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.