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

support-for-publicDNS-for-privateAKScluster : adding private_dns_zone_id & private_cluster_public_fqdn_enabled vars to control private aks cluster #149

Merged
merged 8 commits into from
Jul 13, 2022

Conversation

iamamitgera
Copy link
Contributor

Fixes #000

Changes proposed in the pull request:

Amit Gera and others added 2 commits March 22, 2022 13:19
…ateAKScluster

support-for-publicDNS-for-privateAKScluster : adding private_dns_zone_id & private_cluster_public_fqdn_enabled vars to control private aks cluster
@ghost
Copy link

ghost commented Mar 22, 2022

CLA assistant check
All CLA requirements met.

@stephane-pham-shift
Copy link

Hello,
I have the same requirement.
When can we change it?

main.tf Show resolved Hide resolved
@ghost
Copy link

ghost commented Jun 21, 2022

Any news on when this will be merged?

@lonegunmanb
Copy link
Member

Hi @liechtinat thanks for asking, for now this pr has a tiny format issue. I'd like to wait for @iamamitgera to fix that as he or she is the author, but if you're in a hurry maybe I can submit a new pr based on his to add this feature.

@ghost
Copy link

ghost commented Jun 22, 2022

That would be cool, we currently have a fork of the repo to mitigate this issue as we want to use public resolvable kubeapi records for our private AKS clusters. I can also submit the PR if needed.

@iamamitgera
Copy link
Contributor Author

I am regretful for delay as was occupied somewhere , I will get this done by tomorrow with requested changes

@the-technat
Copy link
Contributor

Looks like terraform fmt was run. Anything that prevents this from merging?

Copy link
Contributor

@the-technat the-technat left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @iamamitgera , almost LGTM but only two tiny description issues.

variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@the-technat
Copy link
Contributor

the-technat commented Jul 8, 2022

@iamamitgera would you mind resolving the description issues?

@mkilchhofer
Copy link
Contributor

@lonegunmanb can we merge this? We can file a PR to fix the quotation if needed but it looks kinda weird to wait on a contributor to fix quotes.

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @iamamitgera , LGTM!

@lonegunmanb lonegunmanb merged commit 577db7d into Azure:master Jul 13, 2022
@mkilchhofer
Copy link
Contributor

🥳 nice.
When do we have this released?

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