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

Rename poorly-named DNS variables #61

Merged
merged 2 commits into from
May 26, 2020
Merged

Conversation

dseeley-sky
Copy link
Contributor

The naming of the variables that describe the DNS zone to which we're adding records (dns_tld_external), the name of the domain part of the FQDN (cluster_vars.dns_zone_external), and the name of the domain part of the cloud-internal FQDN (cluster_vars.dns_zone_internal) is very poor, and leads to misunderstandings.

dns_tld_external  --> cluster_vars.dns_nameserver_zone
dns_zone_internal --> cluster_vars.dns_cloud_internal_domain
dns_zone_external --> cluster_vars.dns_fqdn_domain

The naming of the variables that describe the DNS zone to which we're adding records (dns_tld_external), the name of the domain part of the FQDN (cluster_vars.dns_zone_external), and the name of the domain part of the cloud-internal FQDN (dns_zone_internal) is very poor, and leads to misunderstandings.

dns_tld_external  --> cluster_vars.dns_nameserver_zone
dns_zone_internal --> cluster_vars.dns_cloud_internal_domain
dns_zone_external --> cluster_vars.dns_fqdn_domain
@dseeley-sky
Copy link
Contributor Author

Alternative suggestions welcome...

@eolix eolix self-requested a review May 22, 2020 17:09
eolix
eolix previously approved these changes May 22, 2020
sky-amoncadot
sky-amoncadot previously approved these changes May 26, 2020
Copy link
Contributor

@sky-amoncadot sky-amoncadot left a comment

Choose a reason for hiding this comment

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

LGTM

@leslieonline27
Copy link
Contributor

Not entirely sure I like the way the internal and external diverge naming wise, and they are not technically fqdns? (There is no hostname). Could I suggest the below:

dns_tld_external  --> cluster_vars.dns_nameserver_zone
dns_zone_internal --> cluster_vars.dns_internal_domain_name
dns_zone_external --> cluster_vars.dns_external_domain_name

I think the dns_tld_external rename makes perfect sense (tld might not be entirely clear to some and it's technically incorrect as a description, considering how we've been using it)

@sky-amoncadot
Copy link
Contributor

sky-amoncadot commented May 26, 2020

Not entirely sure I like the way the internal and external diverge naming wise

dns_tld_external  --> cluster_vars.dns_nameserver_zone
dns_zone_internal --> cluster_vars.dns_internal_domain_name
dns_zone_external --> cluster_vars.dns_external_domain_name

Agree with your comment on diverging, however cluster_vars.dns_external_domain_name has external within the naming convention, which suggests to me this should only be used for external dns zones? Currently this variable is used for both internal & external user-defined zones.

they are not technically fqdns? (There is no hostname).

True! In addition to my above suggestion, perhaps cluster_vars.dns_user_domain_name?

dns_tld_external  --> cluster_vars.dns_nameserver_zone
dns_zone_internal --> cluster_vars.dns_internal_domain_name # Default GCP or AWS internal zone 
dns_zone_external --> cluster_vars.dns_user_domain_name # user defined domain and used for internal/external zones

This way there is no divergence across vars and the naming convention for dns vars will always be dns_*.

@leslieonline27
Copy link
Contributor

Currently this variable is used for both internal & external user-defined zones.

Of course, missed that - yep suggestion looks good to me, and I think user is actually a pretty good descriptor

@dseeley-sky
Copy link
Contributor Author

If dns_user_domain_name (which I agree is a good name) is for both internal and external, the dns_internal_domain_name might be a bit confusing?

I can live without the _name suffix for brevity, but not that fussed either way (if people prefer it)? How about:

dns_tld_external  --> cluster_vars.dns_nameserver_zone
dns_zone_internal --> cluster_vars.dns_cloud_internal_domain
dns_zone_external --> cluster_vars.dns_user_domain

@leslieonline27
Copy link
Contributor

If dns_user_domain_name (which I agree is a good name) is for both internal and external, the dns_internal_domain_name might be a bit confusing?

I can live without the _name suffix for brevity, but not that fussed either way (if people prefer it)? How about:

dns_tld_external  --> cluster_vars.dns_nameserver_zone
dns_zone_internal --> cluster_vars.dns_cloud_internal_domain
dns_zone_external --> cluster_vars.dns_user_domain

This LGTM - I'm also ok with _name being dropped for brevity

@antoineserrano antoineserrano self-requested a review May 26, 2020 11:12
@dseeley-sky dseeley-sky dismissed stale reviews from sky-amoncadot and eolix via 40ec1f1 May 26, 2020 13:26
@dseeley-sky dseeley-sky merged commit d544eb7 into master May 26, 2020
@dseeley-sky dseeley-sky deleted the dse09_renamezonetld branch May 26, 2020 16:31
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