Expose modules to support creation of CDP DW Database Catalog and Virtual Warehouses - #19
Expose modules to support creation of CDP DW Database Catalog and Virtual Warehouses#19raju-saravanan wants to merge 8 commits into
Conversation
f8299df to
54d4676
Compare
| cluster_id=dict(required=True, type='str', aliases=['cluster_id']), | ||
| dbc_id=dict(required=False, type='str', aliases=['dbc_id']), | ||
| vw_type = dict(required=False, type='str', aliases=['vw_type']), | ||
| name = dict(required=True, type='str', aliases=['name']), |
There was a problem hiding this comment.
shouldn't be 'name' with an alias of 'name'
| self.target = self.cdpy.sdk.wait_for_state( | ||
| describe_func=self.cdpy.dw.describe_dbc, | ||
| params=dict(cluster_id=self.cluster_id,dbc_id=self.name), | ||
| state='Running', delay=self.delay, timeout=self.timeout |
There was a problem hiding this comment.
state should be self.cdpy.sdk.STARTED_STATES
| self.target = self.cdpy.sdk.wait_for_state( | ||
| describe_func=self.cdpy.dw.delete_vw, | ||
| params=dict(cluster_id=self.cluster_id, vw_id=self.name), | ||
| state='Running', delay=self.delay, timeout=self.timeout |
There was a problem hiding this comment.
self.cdpy.sdk.STARTED_STATES
| self.target = self.cdpy.sdk.wait_for_state( | ||
| describe_func=self.cdpy.dw.describe_vw, | ||
| params=dict(cluster_id=self.cluster_id, vw_id=self.name), | ||
| state='Running', delay=self.delay, timeout=self.timeout |
There was a problem hiding this comment.
self.cdpy.sdk.STARTED_STATES
7e43f33 to
898d275
Compare
1c81d3d to
8122b01
Compare
wmudge
left a comment
There was a problem hiding this comment.
Looks good! I would like to clarify the module names and return values. In addition, the documentation sections need review, esp. the dw_vw.py module. The contents do not reflect the code, and I suspect the format of applicationConfig, for example, needs updating, too.
| ) | ||
|
|
||
| result = DwDbc(module) | ||
| output = dict(changed=False, dbcs=result.dbcs) |
| description: Maximum number of available nodes for Virtual Warehouse autoscaling. | ||
| type: int | ||
| required: False | ||
| config: |
There was a problem hiding this comment.
There is no config parameter group in the code -- commonConfigs, applicationConfigs, ldapGroups, and enableSSO are all "top level" module parameters in the code. Moreover, the parameters are lowercase with underscores.
There was a problem hiding this comment.
My bad, will fix this.
0ac1f4e to
a647c61
Compare
|
@wmudge : Address your comments, can you take another look ? |
wmudge
left a comment
There was a problem hiding this comment.
The documentation and module specification sections need alignment and further detail.
| description: Configurations that are applied to every application in the service. | ||
| type: dict | ||
| required: False | ||
| contains: |
There was a problem hiding this comment.
Apparently, contains has now been replaced by suboptions -- for example: https://github.com/ansible-collections/azure/blob/dev/plugins/modules/azure_rm_securitygroup.py#L49-L52
Full docs are here: https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#documentation-block
| required: False | ||
| contains: | ||
| configBlocks: List of ConfigBlocks for the application. | ||
| type: list |
There was a problem hiding this comment.
Lists require elements, which would be dict in this case, IIRC
| required: False | ||
| content: | ||
| description: Contents of a ConfigBlock. | ||
| type: obj |
| contains: | ||
| keyValues: | ||
| description: Key-value type configurations. | ||
| type: obj |
| - cloudera.cloud.cdp_auth_options | ||
| ''' | ||
|
|
||
| EXAMPLES = r''' |
There was a problem hiding this comment.
Could you add an example or two of using the various nested configurations?
| def main(): | ||
| module = AnsibleModule( | ||
| argument_spec=CdpModule.argument_spec( | ||
| id=dict(required=False, type='str', default=None), |
There was a problem hiding this comment.
Only use default when necessary. The _get_param() should handle a non-existent parameter.
There was a problem hiding this comment.
Same with required -- only needed if True
| autoscaling_min_nodes=dict(required=False, type='int', default=None), | ||
| autoscaling_max_nodes=dict(required=False, type='int', default=None), | ||
| common_configs=dict(required=False, type='dict', default=None), | ||
| application_configs=dict(required=False, type='dict', default=None), |
There was a problem hiding this comment.
Both common_configs and application_configs need to be expanded to match documentation rules above.
| ) | ||
|
|
||
| result = DwVw(module) | ||
| output = dict(changed=False, virtual_warehouses=result.virtual_warehouses) |
There was a problem hiding this comment.
changed is invalid -- this module can make changes.
4213ebb to
5d07cde
Compare
|
@wmudge : I have expanded the Here "das-webapp" is dynamic key, we can have configs for other services as well. |
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
Signed-off-by: Saravanan Raju <saravanan.footloose@gmail.com>
5d07cde to
7032e26
Compare
|
Superseded by #29 |
Changes in this PR:
Changes in this PR are done in conjunction with the following PRs: