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

Respect $KUBECONFIG env var #68

Merged
merged 4 commits into from
Dec 20, 2017
Merged

Conversation

pcalcado
Copy link
Contributor

@pcalcado pcalcado commented Dec 19, 2017

This PR extracts some of the kube api creatin logic into its own object, and makes sure that it respects the $KUBECONFIG variable. It would probably be more interesting to use something like Kubernetes' own logic to deal with this, but this is not in scope for this issue.

Closes #17

@pcalcado pcalcado self-assigned this Dec 19, 2017
@pcalcado pcalcado force-pushed the phil/should-respect-KUBECONFIG-env branch from a12cd76 to ff53a10 Compare December 19, 2017 21:48
@pcalcado pcalcado force-pushed the phil/should-respect-KUBECONFIG-env branch from ff53a10 to af761b4 Compare December 19, 2017 22:13
@klingerf klingerf force-pushed the phil/should-respect-KUBECONFIG-env branch from af761b4 to 5eec58e Compare December 19, 2017 22:26
@pcalcado pcalcado force-pushed the phil/should-respect-KUBECONFIG-env branch 2 times, most recently from e91ad2a to f35c57d Compare December 19, 2017 22:50
Copy link
Member

@klingerf klingerf 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. I just had some small style nits.

home := shell.HomeDir()
expected := os.Getenv("HOME")
if runtime.GOOS != "windows" && !strings.Contains(home, expected) {
t.Errorf("This is a UNIX-like system, expecting home dir [%s] to contain [/home]", expected)
Copy link
Member

Choose a reason for hiding this comment

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

I think this error would make more sense as:

t.Errorf("This is a UNIX-like system, expecting home dir [%s] to contain [%s]", home, expected)

Copy link
Member

@hawkw hawkw Dec 20, 2017

Choose a reason for hiding this comment

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

unix system

cli/k8s/api.go Outdated

"github.com/runconduit/conduit/cli/shell"
"k8s.io/client-go/rest"
)
Copy link
Member

Choose a reason for hiding this comment

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

Mind collapsing the blank lines here and re-saving with goimports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, I configured IntelliJ to use goimports upon save automatically

}

if actualUrl.String() != expectedUrlString {
t.Fatalf("Expected generated URl to be [%s], but got [%s]", expectedUrlString, actualUrl.String())
Copy link
Member

Choose a reason for hiding this comment

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

typo here: URl => URL

}
})

t.Run("Return error it extra path doesn'' start with slash", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

typo here: doesn'' => doesn't

@pcalcado pcalcado force-pushed the phil/should-respect-KUBECONFIG-env branch from f35c57d to d50c7d6 Compare December 20, 2017 00:44
@pcalcado pcalcado merged commit 0a6a9ed into master Dec 20, 2017
@pcalcado pcalcado deleted the phil/should-respect-KUBECONFIG-env branch December 20, 2017 00:50
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this pull request Mar 5, 2019
In order to start dismantling the monolithic ctx structures, this change
removes the root `ctx::Process` type. This simplifies `ctx::Proxy` such
that is copyable and need not be `Arc`ed.

`telemetry::metrics::labels::Direction` has been updated to decorate
`ctx::Proxy` instead of modeling the same variants directly as an enum.
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this pull request Mar 5, 2019
Following linkerd#67 and linkerd#68, the `labels::Direction` type can be replaced with
an `impl FmtLabels for ctx::Proxy`.
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this pull request Mar 5, 2019
Following linkerd#67 and linkerd#68, the `labels::TlsStatus` type can be removed in
favor of extending underlying `ctx::transport::TlsStatus` type to
implement `FmtLabels`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants