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

Add page about kube-context handling in docs concepts section #2992

Conversation

corneliusweig
Copy link
Contributor

Relates to #2510 and #2447
Also see https://github.com/GoogleContainerTools/skaffold/blob/master/docs/design_proposals/configurable-kubecontext.md

Description

This PR adds a page to the concepts section in the docs about the kube-context activation. The PR also addresses a comment from #2510 (review).

User facing changes
n/a
Before
n/a
After
n/a
Next PRs.
n/a
Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Mentions any output changes.
  • Adds documentation as needed: user docs, YAML reference, CLI reference.
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Oct 6, 2019

Codecov Report

Merging #2992 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted Files Coverage Δ
pkg/skaffold/build/custom/custom.go 62.5% <0%> (-1.44%) ⬇️
pkg/skaffold/runner/diagnose.go 12.67% <0%> (-0.76%) ⬇️
pkg/skaffold/kubernetes/context/context.go 100% <0%> (ø) ⬆️
pkg/skaffold/build/misc/graceful.go 66.66% <0%> (ø)
pkg/skaffold/kubernetes/status_check.go 77.77% <0%> (ø)
pkg/skaffold/schema/profiles.go 88.5% <0%> (+0.06%) ⬆️
pkg/skaffold/schema/defaults/defaults.go 92% <0%> (+0.91%) ⬆️
pkg/skaffold/build/gcb/types.go 16.12% <0%> (+1.42%) ⬆️
pkg/skaffold/docker/dependencies.go 73.91% <0%> (+2.17%) ⬆️
pkg/skaffold/build/custom/dependencies.go 83.33% <0%> (+8.33%) ⬆️

@corneliusweig
Copy link
Contributor Author

cc @balopat as you raised this in #2510 (review)

@balopat balopat added the docs-modifications runs the docs preview service on the given PR label Oct 9, 2019
@container-tools-bot
Copy link

Please visit http://35.236.111.216:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Oct 9, 2019
tejal29
tejal29 previously requested changes Oct 9, 2019
docs/content/en/docs/concepts/kube_context/_index.md Outdated Show resolved Hide resolved

When interacting with a kubernetes cluster, Skaffold does so via a kube-context.
Thus, the selected kube-context determines the kubernetes cluster, the kubernetes user, and the default namespace.
By default, Skaffold uses the _current_ kube-context from your kube-config file.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By default, Skaffold uses the _current_ kube-context from your kube-config file.
By default, Skaffold uses the _current_ kube-context from your kube-config file at `~/.kube/config`

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 also thought about including this, but Skaffold also respects the KUBECONFIG env var. So the location you suggest isn't always correct. I'm fine either way, but please just confirm that you really want the path there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should leave the path out, since this file doesn't necessarily have to live anywhere.

docs/content/en/docs/concepts/kube_context/_index.md Outdated Show resolved Hide resolved

To override this default, Skaffold offers two options:

1. Via the `kube-context` flag
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Via the `kube-context` flag
1. `kube-context` flag

docs/content/en/docs/concepts/kube_context/_index.md Outdated Show resolved Hide resolved
docs/content/en/docs/concepts/kube_context/_index.md Outdated Show resolved Hide resolved
- kubeContext: minikube
```

It is illegal to activate both profiles which happens when
Copy link
Member

Choose a reason for hiding this comment

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

you could re-word this as

Suggested change
It is illegal to activate both profiles which happens when
Activating multiple `kube-contexts` will result in error.

You could also mention the error we print here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wording sounds as if it is illegal to have multiple differing profiles.deploy.kubeContext configurations. However that is not the case.
What I am trying to say is that it is illegal to have automatic profile activations via a matching kube-context AND the effective kube-context changes.

I'll try to make that point clearer.


### Kube-context activation and Skaffold profiles

The kube-context has a double role for Skaffold profiles:
Copy link
Member

Choose a reason for hiding this comment

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

This is a little bit confusing.
You could say,

Suggested change
The kube-context has a double role for Skaffold profiles:
Multiple `kube-contexts` can be activated from skaffold profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A put some thoughts into this, if I can make this whole part clearer. Can you have another look?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes sense. the main point here is that the kube-context has two semantic roles in the activation of skaffold profiles, but not that multiple contexts can be activated from profiles (since that's not really true).

docs/content/en/docs/concepts/kube_context/_index.md Outdated Show resolved Hide resolved
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig
Copy link
Contributor Author

Hey @tejal29, thanks for doing such a thorough check of this docs change. I tried to improve the readability and logic flow. Can you take another look?

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig
Copy link
Contributor Author

That CI failure seems unrelated.

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for clarifying this @corneliusweig

@nkubala nkubala added the kokoro:run runs the kokoro jobs on a PR label Oct 11, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Oct 11, 2019
@nkubala nkubala added the kokoro:run runs the kokoro jobs on a PR label Oct 11, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Oct 11, 2019
@nkubala nkubala merged commit ad23bcd into GoogleContainerTools:master Oct 11, 2019
@corneliusweig corneliusweig deleted the w/kubecontext/update-concepts-docs branch October 11, 2019 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants