gcp_compute: use env variables on inventory script #52685
Conversation
|
@sivel please take a look when you can! |
|
|
||
| def fail_json(self, *args, **kwargs): | ||
| raise AnsibleError(kwargs['msg']) | ||
|
|
||
| def _set_fallbacks(self): | ||
| self._set_fallback('project', 'GCP_PROJECT') |
There was a problem hiding this comment.
Plugins can use documentation as an arg spec and validator (e.g. if someone doesn't provide an option with required: True or provides an option of the wrong type they'll get a helpful error without the plugin itself needing to do that). You can also specify options that fall back to environment variables without having to retrieve those yourself with os.environ.
To do that you should document your options like this:
projects:
description: A list of projects in which to describe GCE instances.
env:
- name: GCP_PROJECT
...
I also think pulling in the gcp doc fragment is a little confusing. Do you really want the plugin to be able to use the option project and projects? Some of the other options are now duplicated. And it doesn't list environment variables in a way the plugin can use. In the case of AWS credentials, a new documentation fragment was created (aws_credentials) for plugins specifically. GCP may want to do the same if you find yourself duplicating documentation between plugins.
Looking at the rest of the plugin, I notice there's some unnecessary validation happening as well that should be put in the documentation/argspec. A plugin doesn't need to do any of this:
# get user specifications
if 'zones' in config_data:
if not isinstance(config_data['zones'], list):
raise AnsibleParserError("Zones must be a list in GCP inventory YAML files")
# get user specifications
if 'projects' not in config_data:
raise AnsibleParserError("Projects must be included in inventory YAML file")
if not isinstance(config_data['projects'], list):
raise AnsibleParserError("Projects must be a list in GCP inventory YAML files")
# add in documented defaults
if 'filters' not in config_data:
config_data['filters'] = None
just use type: list or required: True for those options. Then reference your options with self.get_option(option_name) rather than config_data[option_name], since the latter will not set the default (None unless otherwise specified, like module parameters) or fall back to environment variables if you have one documented.
There's some documentation here: https://docs.ansible.com/ansible/latest/dev_guide/developing_plugins.html#plugin-configuration-documentation-standards. Also feel free to ping me (shertel) on IRC if you have questions.
|
This PR can be closed, because it should be resolved with #57776 |
|
Closing as per above, please reopen if I've missed something. |
SUMMARY
Use environment variables on gcp_compute inventory script.
ISSUE TYPE
COMPONENT NAME
gcp_compute
ADDITIONAL INFORMATION