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

Provide compatibility for dns_zone module with openstack.cloud collection #105

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Polina-Gubina
Copy link
Contributor

@Polina-Gubina Polina-Gubina commented May 4, 2021

As a user of OTC switching between openstack module and otc module in the current form requires changes in the parameters (zone_type=private is not supported by openstack vs zone_type=public not supported by OTC).
As such we should have following:

  • zone_type is present for compatibility with OS but is ignored
  • visibility parameter added with private/public values for the OTC specifics

tischrei
tischrei previously approved these changes May 4, 2021
otc-zuul[bot]
otc-zuul bot previously approved these changes May 4, 2021
SebastianGode
SebastianGode previously approved these changes May 5, 2021
@@ -47,13 +47,24 @@
description:
- Cache duration (in second) on a local DNS server
type: int
zone_type:
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'type' means whether primary or secondary dns zone (line 56), did you mean alias 'type'?

Copy link
Member

Choose a reason for hiding this comment

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

no. Its actually a recordset that is in openstack has "recordset_type", and on our side "type". For that we should on our side rename "type" attribute into "recordset_type" and with aliases: ['type'] provide backward compatibility.

Later: Well, we actually need to drop recordset module completely and only provide routing to the openstack one

Copy link
Member

Choose a reason for hiding this comment

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

let's rename it not to type, but "visibility"

tischrei
tischrei previously approved these changes May 10, 2021
@@ -47,13 +47,24 @@
description:
- Cache duration (in second) on a local DNS server
type: int
zone_type:
Copy link
Member

Choose a reason for hiding this comment

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

no. Its actually a recordset that is in openstack has "recordset_type", and on our side "type". For that we should on our side rename "type" attribute into "recordset_type" and with aliases: ['type'] provide backward compatibility.

Later: Well, we actually need to drop recordset module completely and only provide routing to the openstack one

@@ -113,7 +113,7 @@
- dns_fl.ptr.description is defined

- name: Creating a public DNS Zone - check mode
opentelekomcloud.cloud.dns_zones:
Copy link
Member

Choose a reason for hiding this comment

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

hmm, how was this working before? Weird

- This parameter is disabled, it only provides compatibility with openstack.cloud colection.
choices: [primary, secondary]
type: str
masters:
Copy link
Member

Choose a reason for hiding this comment

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

you add "masters", but do not use it

@@ -47,13 +47,24 @@
description:
- Cache duration (in second) on a local DNS server
type: int
zone_type:
Copy link
Member

Choose a reason for hiding this comment

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

let's rename it not to type, but "visibility"

description:
- Zone Type, either public or private
type: str
choices: [public, private]
default: public

type:
Copy link
Member

Choose a reason for hiding this comment

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

must be zone_type (as currently in openstack module)

@@ -95,8 +106,8 @@
description: Cache duration (in second) on a local DNS server
type: int
sample: 300
zone_type:
Copy link
Member

Choose a reason for hiding this comment

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

I think when we return result we need to explicitly rename zone_type to visibility

@@ -107,7 +118,7 @@
opentelekomcloud.cloud.dns_zone:
name: "test.com."
state: present
zone_type: private
Copy link
Member

Choose a reason for hiding this comment

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

zone_type: primary
visibility: private

email=dict(required=False),
name=dict(required=True),
router=dict(required=False),
state=dict(type='str', choices=['present', 'absent'], default='present'),
ttl=dict(required=False, type='int'),
zone_type=dict(type='str', choices=['public', 'private'], default='public')
masters=dict(required=False, type='list', elements='str'),
type=dict(required=False, choices=['primary', 'secondary'], type='str')
Copy link
Member

Choose a reason for hiding this comment

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

this is "visibilty"
and zone_type parameter as such is still described here, hust not used further in the code

@@ -135,7 +148,7 @@ def run(self):
changed = False
attrs = {}
query = {
'type': self.params['zone_type'],
'type': self.params['type'],
Copy link
Member

Choose a reason for hiding this comment

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

'zone_type': self.params['visibilty'],

@@ -170,7 +183,7 @@ def run(self):
self.exit_json(changed=True)
if not needs_update:
# Check if VPC exists
if self.params['zone_type'] == 'private':
if self.params['type'] == 'private':
Copy link
Member

Choose a reason for hiding this comment

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

visibilty

if self.params['zone_type']:
attrs['zone_type'] = self.params['zone_type']

if self.params['type']:
Copy link
Member

Choose a reason for hiding this comment

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

                if self.params['visibility']:
                    attrs['zone_type'] = self.params['visibility']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants