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

Allow passing relative root when running inCluster #383

Merged
merged 5 commits into from
Dec 29, 2018

Conversation

olegsu
Copy link
Contributor

@olegsu olegsu commented Dec 27, 2018

related to #382

Copy link
Contributor

@silasbw silasbw left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks for the PR. Two comments around naming and semantics.

lib/config.js Outdated
@@ -32,16 +32,16 @@ function defaultConfigPaths () {
function getInCluster () {
const host = process.env.KUBERNETES_SERVICE_HOST
const port = process.env.KUBERNETES_SERVICE_PORT

const root = process.env.RELATIVE_ROOT || '/'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pick a kubernetes-client specific name for this environment variable? How about KUBERNETES_CLIENT_SERVICEACCOUNT_ROOT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will do asap

lib/config.js Outdated
@@ -32,16 +32,16 @@ function defaultConfigPaths () {
function getInCluster () {
const host = process.env.KUBERNETES_SERVICE_HOST
const port = process.env.KUBERNETES_SERVICE_PORT

const root = process.env.RELATIVE_ROOT || '/'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also set the default to /var/run/secrets/kubernetes.io/serviceaccount/ and update caPath, tokenPath, and namespacePath to be like ${ KUBERNETES_CLIENT_SERVICEACCOUNT_ROOT }/*? I'm guessing that would implement the behavior folks have in mind when they set the KUBERNETES_CLIENT_SERVICEACCOUNT_ROOT value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I fully understand, can you please give an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

The updates you made were what I had in mind. 👍

@olegsu
Copy link
Contributor Author

olegsu commented Dec 29, 2018

@silasbw please check this out.
I've updated the name to be KUBERNETES_CLIENT_SERVICEACCOUNT_ROOT.
So it supports my example as:

export KUBERNETES_CLIENT_SERVICEACCOUNT_ROOT=$TELEPRESENCE_ROOT/var/run/secrets/kubernetes.io/serviceaccount/

Copy link
Contributor

@silasbw silasbw left a comment

Choose a reason for hiding this comment

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

🥇 🌶️

@silasbw silasbw merged commit 623453d into godaddy:master Dec 29, 2018
@silasbw
Copy link
Contributor

silasbw commented Dec 29, 2018

Published your change with kubernetes-client@6.7.0.

@olegsu
Copy link
Contributor Author

olegsu commented Dec 29, 2018

Awesome, thanks

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.

None yet

2 participants