Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Added example kubernetes yaml #39

Merged
merged 2 commits into from
Jun 1, 2018
Merged

Added example kubernetes yaml #39

merged 2 commits into from
Jun 1, 2018

Conversation

jimangel
Copy link
Contributor

I hacked my way through figuring this out and thought it might be helpful to others. Thanks!

I hacked my way through figuring this out and thought it might be helpful to others. Thanks!
@codecov
Copy link

codecov bot commented May 30, 2018

Codecov Report

Merging #39 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #39   +/-   ##
=======================================
  Coverage   39.42%   39.42%           
=======================================
  Files           1        1           
  Lines         208      208           
=======================================
  Hits           82       82           
  Misses        121      121           
  Partials        5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80ce8a7...033aa71. Read the comment docs.

Copy link
Owner

@negz negz left a comment

Choose a reason for hiding this comment

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

Great idea! I'm surprised I got this far without an example deployment. I added a few small comments - what do you think?

README.md Outdated
secret: ZXhhbXBsZS1hcHAtc2VjcmV0
---
apiVersion: v1
kind: Service
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps we should leave the service as an exercise for the reader? I feel like these things vary too much from company to company (i.e. some might use an ingress controller, others type: NodePort, others type: LoadBalancer) for an example to provide much illustration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I haven't done a deep dive on your code to see if there was any issues with a cloud LB or doing something like SSL termination on an ingress controller. If it is a simple client / server model, and you see no issues with it, I think it makes sense to leave it up to the operator and just notate that.

README.md Outdated
mountPath: /cfg
dnsConfig:
nameservers:
- 8.8.8.8 # USING GOOGLE DNS FOR LOOKUPS
Copy link
Owner

Choose a reason for hiding this comment

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

Is this something that commonly needs to be set? I don't think I've ever explicitly configured my pods' nameserver, but rather have let them use KubeDNS.

Copy link
Contributor Author

@jimangel jimangel May 31, 2018

Choose a reason for hiding this comment

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

I don't think it is common, but I ran into some funky things using an air-gapped k8s cluster. Ok with removing dnsConfig from the example. For the hostAliases / hostnames, I think it's worth leaving in. In my case, I had the hardest time getting kuberos pod to create an OIDC client (i/o time out). When it can't create the client, it crashes and the pod restarts (hard to find logs as it's flapping). I eventually updated the dex endpoint with a valid cert but not a public DNS record and the hostAliases "hack" fixed that.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good - I agree there's value in the hostAliases.

README.md Outdated
@@ -112,6 +112,88 @@ If the `current-context` is set to the name of one of the clusters then the
`--context` argument may be omitted, and the cluster named by `current-context`
will be used.

## Kubernetes (example)
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps title this "Deploying to Kubernetes"?

Copy link
Contributor Author

@jimangel jimangel left a comment

Choose a reason for hiding this comment

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

I agree with your direction, left some insight on my "whys." Thanks!

@negz negz merged commit 14f4ca7 into negz:master Jun 1, 2018
@jimangel jimangel deleted the patch-1 branch May 9, 2019 18:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants