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

Initial commit for Oracle Cloud Infrastructure modules #53156

Open
wants to merge 1 commit into
base: devel
from

Conversation

@nalsaber
Copy link

nalsaber commented Mar 1, 2019

SUMMARY

Initial commit for Oracle cloud infrastructure

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

Oracle cloud infrastructure Manage Virtual Cloud Networks(VCN) module

ADDITIONAL INFORMATION

Files Submitted:
lib/ansible/module_utils/oracle/init.py
lib/ansible/module_utils/oracle/oci_logging.yaml
lib/ansible/module_utils/oracle/oci_utils.py
lib/ansible/modules/cloud/oracle/init.py
lib/ansible/modules/cloud/oracle/oci_vcn.py
lib/ansible/plugins/doc_fragments/oracle.py
lib/ansible/plugins/doc_fragments/oracle_creatable_resource.py
lib/ansible/plugins/doc_fragments/oracle_display_name_option.py
lib/ansible/plugins/doc_fragments/oracle_name_option.py
lib/ansible/plugins/doc_fragments/oracle_tags.py
lib/ansible/plugins/doc_fragments/oracle_wait_options.py

@nalsaber nalsaber force-pushed the nalsaber:oracle-modules branch from 22fbcd3 to 893407c Mar 5, 2019

api_user_fingerprint=dict(type="str", required=False, no_log=True),
api_user_key_file=dict(type="str", required=False),
api_user_key_pass_phrase=dict(type="str", required=False, no_log=True),
auth_type=dict(

This comment has been minimized.

@gundalow

gundalow Mar 8, 2019

Contributor

Do you need this?
Could you just see if api_key is set or not?

This comment has been minimized.

@rohitChaware

rohitChaware Mar 12, 2019

There may be a case when api_key is set but still an user wants to authenticate using instance principal by setting auth_type to instance_principal.

# method, ensure that no `oci` python sdk dependencies are introduced in this method. This ensures that the modules
# can check for absence of OCI Python SDK and fail with an appropriate message. Introducing an OCI dependency in
# this method would break that error handling logic.
common_args = dict(

This comment has been minimized.

@gundalow

gundalow Mar 8, 2019

Contributor

Please remove required=False that's the default

This comment has been minimized.

@nalsaber
)

if not HAS_OCI_PY_SDK:
module.fail_json(msg=missing_required_lib("OCI Python SDK"))

This comment has been minimized.

@gundalow

gundalow Mar 8, 2019

Contributor

I think this should be the name of the library

This comment has been minimized.

@nalsaber

nalsaber Mar 8, 2019

Author

The library is called oci-python-sdk

return True


def has_user_provided_value_for_option(module, option):

This comment has been minimized.

@gundalow

gundalow Mar 8, 2019

Contributor

The aliases should be available via the standard name. I'm not sure what this function is for

ie

display_name=dict(type="str", required=False, aliases=["name"]),
foo = module.params["display_name"]
# foo would be set to the what's passed in as `display_name` or `name`

This comment has been minimized.

@rohitChaware

rohitChaware Mar 12, 2019

The function has_user_provided_value_for_option() figures out if an user has actually provided value for option using standard name or alias. This helps in preparing the list of attributes of a resource which need to be updated in the function get_attr_to_update().

module.params has all the standard options set to some value(user provided, default or None) irrespective of whether user provided it or not.

This comment has been minimized.

@felixfontein

felixfontein Mar 12, 2019

Contributor

But why to do you need to figure out whether the option was provided via the main name or an alias? No other module makes any distinction by which name the option values are passed AFAIK. So why break with this convention?

This comment has been minimized.

@rohitChaware

rohitChaware Mar 12, 2019

Thanks @felixfontein for reviewing this closely.

We have generalized the update utility[check_and_update_resource()] which is used by all the OCI Ansible modules for update scenarios.

To have a consistent design, the names of attributes of the update model of a resource become 'options with main name or alias' in the Ansible module for the resource.

For example, attributes: defined_tags, display_name, freeform_tags of the update model: https://oracle-cloud-infrastructure-python-sdk.readthedocs.io/en/latest/api/core/models/oci.core.models.UpdateVcnDetails.html are options in oci_vcn Ansible module and attribute:display_name could be alias for an option:name.

Then while iterating over the update model of a resource to figure out if the value of an attribute is actually provided by the user, we look for this attribute name in main names or aliases. If we don’t look in aliases, we may miss out as in the following case:

display_name could be alias for an option having standard name as name. While iterating on the update model to check if value of attribute:display_name is provided, we have to look for it in aliases too if user has used option:name to specify the value since params["name"] will be set and not params["display_name"].

This comment has been minimized.

@felixfontein

felixfontein Mar 12, 2019

Contributor

If name is an alias of display_name, and the user sets name: xxx, params['display_name'] will be 'xxx'. Ansible does that internally so no module has to do that.

This comment has been minimized.

@rohitChaware

rohitChaware Mar 12, 2019

Agree and it is true for module.params.

In has_user_provided_value_for_option(), we are processing return value of _load_params(), which is raw user input dictionary and has not gone through the default value substitution and alias processing.

We need raw user input so that we can distinguish between cases when (a) a user explicitly assigns a value to an option and (b) a value gets assigned because of default value for the option.

Once we know what options are explicitly specified by a user, we update only those attributes.

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

I think the standard way to do this (i.e. what all other modules do) is to not specify a default value for that option, and use None as "was not specified by user". I don't know how the core team sees this. If modules should be allowed to know where a value comes from (user, default value, environment, ...), I think there should be a common mechanism which can be used by all modules.

This comment has been minimized.

@nalsaber

nalsaber Mar 15, 2019

Author

@sivel @samdoran hey guys do you have any input on this?

This comment has been minimized.

@samdoran

samdoran Mar 18, 2019

Member

This feels a lot like reimplementing argument spec validation methods inside a module. I would advise against this.

We need raw user input so that we can distinguish between cases when (a) a user explicitly assigns a value to an option and (b) a value gets assigned because of default value for the option.

I don't fully understand this need. The module should not care if a user provided the value or if it came from the default setting in the argument spec. It should only care about the value. Treating the value differently based on where it was set, particularly if the provided value is the same as the default value, seems overly complicated.

This comment has been minimized.

@rohitChaware

rohitChaware Mar 20, 2019

Thanks @felixfontein and @samdoran for your inputs.

We have a few module options in some OCI Ansible modules which are common to create and update operations. These options have defaults during create operation(as API puts in default value during create operation if nothing is specified) but don't have any defaults for update operation.

As suggested under options:->default: on https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#documentation-fields, we preferred mentioning default values for these options using the default construct instead of adding to option's documentation about defaults.

So during update operation when a user hasn't specified a value for such an option, the option gets substituted by the default value. Thus we have to check if the user provided the value or it came from the default setting in argument spec before we actually proceed for the update operation. If we don't make this check, we will end up updating the attribute of the resource even if the user didn't intend to.

@gundalow gundalow modified the milestone: s Mar 8, 2019

)

if not HAS_OCI_PY_SDK:
module.fail_json(msg=missing_required_lib("OCI Python SDK"))

This comment has been minimized.

@gundalow

gundalow Mar 8, 2019

Contributor
Suggested change
module.fail_json(msg=missing_required_lib("OCI Python SDK"))
module.fail_json(msg=missing_required_lib("oci-python-sdk"))

This comment has been minimized.

@felixfontein

felixfontein Mar 11, 2019

Contributor

I think it is just called oci: https://pypi.org/project/oci/ At least https://pypi.org/project/oci-python-sdk/ does not exist.

This comment has been minimized.

@nalsaber

nalsaber Mar 15, 2019

Author

I will rename it to 'oci'

@ansibot ansibot added the stale_ci label Mar 15, 2019

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.