-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Improve Azure tutorials #3201
Improve Azure tutorials #3201
Conversation
ba18ffe
to
14d52c5
Compare
docs/tutorials/azure-private-dns.md
Outdated
./nginx-ingress-controller --publish-service=default/nginx-ingress-controller | ||
``` | ||
|
||
## Expose an nginx service with an ingress (Optional) | ||
|
||
Create a service file called 'nginx.yaml' with the following contents: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this redundant with the Deployment
deployed above? I'd think this would get cut down to just deploying the Ingress
resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The service
resource is also different here because ClusterIP
is enough, but in the previous snippet the service is of type LoadBalancer
.
I am proposing now a new version to avoid duplication of yaml code. Please take a look again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @zioproto for improving our docs, much appreciated ❤️.
When you have time could you try to address the comments from @johngmyers.
If everything is good I'd love to merge your PR.
7e9f195
to
2ef00e3
Compare
* Bump container images to v0.13.1 * Add example with kubernetes service and `external-dns.alpha.kubernetes.io/hostname` annotation * Add example with `external-dns.alpha.kubernetes.io/internal-hostname` * Add example used in combination with `service.beta.kubernetes.io/azure-load-balancer-internal`
2ef00e3
to
617a32a
Compare
@johngmyers please take a look again. I pushed new commits that address your comments. Thanks |
/lgtm |
/assign @njuettner |
@njuettner friendly ping. Can we merge ? thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njuettner, zioproto The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Improves the Azure Tutorials
external-dns.alpha.kubernetes.io/hostname
annotationexternal-dns.alpha.kubernetes.io/internal-hostname
service.beta.kubernetes.io/azure-load-balancer-internal