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

feat(config): support multiple config files in KUBECONFIG #326

Merged
merged 7 commits into from
Sep 10, 2018
Merged

feat(config): support multiple config files in KUBECONFIG #326

merged 7 commits into from
Sep 10, 2018

Conversation

JacopoDaeli
Copy link
Contributor

@JacopoDaeli JacopoDaeli commented Sep 6, 2018

Adding support for multiple config files in KUBECONFIG env variable.
Official ref: https://kubernetes.io/docs/tasks/access-application-cluster/configure-access-multiple-clusters/#create-a-second-configuration-file

Output of console.log(yaml.dump(merge.all(configs))) (ref to configs) is:
image
which matches the output of kubectl config view:
image

@JacopoDaeli
Copy link
Contributor Author

JacopoDaeli commented Sep 6, 2018

Follow up after discussion in #322.

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.

This looks great.

Can you add some tests?

@JacopoDaeli
Copy link
Contributor Author

Of course. I will deal with it ASAP.

Copy link
Collaborator

@jcrugzz jcrugzz 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 @silasbw. Looks good but tests would be 💯

@JacopoDaeli
Copy link
Contributor Author

@jcrugzz @silasbw tests added.

lib/config.js Outdated

return config;
}

function loadKubeconfig(cfgPath) {
cfgPath = cfgPath || defaultConfigPath();
const cfgPaths = cfgPath ? [cfgPath] : defaultConfigPath();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JacopoDaeli was thinking, should we support passing an array of config paths in here as well? i.e.

let cfgPaths;
if (!cfgPath) { cfgPaths = defaultConfigPath(); }
else if(Array.isArray(cfgPath)) { cfgPaths = cfgPath; }
else { cfgPaths = [cfgPath]; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that too. Not sure. In any case, that doesn't hurt, but is it useful?
@silasbw what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm 👍

Copy link
Collaborator

@jcrugzz jcrugzz left a comment

Choose a reason for hiding this comment

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

@JacopoDaeli LGTM, an extra test for the new API possibility could be nice but I won't block it here.

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.

nit: rename defaultConfigPath to defaultConfigPaths

@silasbw silasbw merged commit d76a84f into godaddy:master Sep 10, 2018
@JacopoDaeli JacopoDaeli deleted the jdaeli/deepConfigMerge branch September 10, 2018 21:35
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

3 participants