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

Improve error messages for deploy.kubeContext error cases #2993

Conversation

corneliusweig
Copy link
Contributor

Relates to #2510 (review)

Description

Improve error logging for two error cases which may happen when configuring the kube-context via skaffold.yaml

  • Improve error message when activating conflicting Skaffold profiles
  • Warn when changing the kube-context of a running Skaffold process

In addition, include the newly added example/profiles in integration tests (I assumed that all examples are automatically tested, but this is not the case).

User facing changes

  1. after When changing deploy.kubeContext of a running Skaffold process, a warn-level log message is printed, saying

    Changing the kube-context is not supported after startup. Please restart Skaffold to take effect.

  2. before When trying to activate conflicting Skaffold profiles, there used to be an error with the message

    some activated profile contains kubecontext specific settings for a different than the effective kubecontext, please revise your profile activations

    after This log message was improved to include some more information about the initial and resulting kube-context, and what are the context-specific profiles which got activated by the initial kube-context, e.g.:

    profiles ["minikube-profile"] were activated by kube-context "minikube", but the effective kube-context is "staging" -- please revise your profile activations

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.

See the contribution guide for more details.

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>
Also, fix config version of the profiles example.

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
Before, the error did not specify which profiles were activated and
what is the initial or effective kube-context.

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

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

@codecov
Copy link

codecov bot commented Oct 6, 2019

Codecov Report

Merging #2993 into master will increase coverage by 0.04%.
The diff coverage is 94.44%.

Impacted Files Coverage Δ
pkg/skaffold/kubernetes/context/context.go 100% <100%> (ø) ⬆️
pkg/skaffold/schema/profiles.go 88.5% <91.66%> (+0.06%) ⬆️
pkg/skaffold/build/custom/custom.go 62.5% <0%> (-1.44%) ⬇️
pkg/skaffold/runner/diagnose.go 12.67% <0%> (-0.76%) ⬇️
pkg/skaffold/build/misc/graceful.go 66.66% <0%> (ø)
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%) ⬆️

}

func activatedProfiles(profiles []latest.Profile, opts cfg.SkaffoldOptions) ([]string, bool, error) {
func activatedProfiles(profiles []latest.Profile, opts cfg.SkaffoldOptions) ([]string, []string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this now returns a list of profiles and activated profile?

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 quite. It returns a list of activated profiles and a list of activated profiles that are kube-context specific. I added a godoc to clarify this.

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

@tejal29 Thanks taking the time to look at this! At your convenience please take another look.

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Oct 8, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Oct 8, 2019
@tejal29 tejal29 merged commit 2810eee into GoogleContainerTools:master Oct 9, 2019
@corneliusweig corneliusweig deleted the w/kubecontext/improve-errors branch October 9, 2019 19:53
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.

4 participants