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

Use configured namespace for pod watcher. #1473

Merged
merged 8 commits into from Jan 30, 2019

Conversation

michaelfig
Copy link
Contributor

This is a first step towards working on my on-premises cluster.

Basically, the pod watcher was failing because it requests cluster-wide pod access, but my RBAC for the current user restricts it to just the michael namespace.

An enhancement would be to use the value of the --namespace flag to override the context's namespace if supplied.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #1473 into master will increase coverage by 0.29%.
The diff coverage is 50.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1473      +/-   ##
==========================================
+ Coverage   45.62%   45.91%   +0.29%     
==========================================
  Files         116      118       +2     
  Lines        4870     4937      +67     
==========================================
+ Hits         2222     2267      +45     
- Misses       2424     2440      +16     
- Partials      224      230       +6
Impacted Files Coverage Δ
pkg/skaffold/kubernetes/watcher.go 0% <0%> (ø) ⬆️
pkg/skaffold/kubernetes/log.go 6.55% <0%> (-0.17%) ⬇️
pkg/skaffold/runner/dev.go 56.52% <100%> (ø) ⬆️
pkg/skaffold/kubernetes/port_forward.go 42.27% <28.57%> (-0.23%) ⬇️
pkg/skaffold/runner/runner.go 60.94% <70.96%> (-0.54%) ⬇️
pkg/skaffold/sync/sync.go 91.42% <88.88%> (+0.12%) ⬆️
pkg/skaffold/deploy/kubectl/cli.go 0% <0%> (ø) ⬆️
pkg/skaffold/schema/versions.go 65.85% <0%> (ø) ⬆️
pkg/skaffold/schema/v1beta3/config.go 100% <0%> (ø)
pkg/skaffold/schema/v1beta3/upgrade.go 58.62% <0%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3084ca3...6c08f07. Read the comment docs.

@michaelfig
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@r2d4
Copy link
Contributor

r2d4 commented Jan 23, 2019

Skaffold can deploy namespaced objects as well - I think to be really correct you'd need to get the namespace (if specified) of the deployed resources as well as the default, or passed-through namespace. But this makes sense as a first step

@michaelfig
Copy link
Contributor Author

michaelfig commented Jan 23, 2019

@r2d4 Yes absolutely. I'll quote from #1521:

However, the skaffold -n option and SKAFFOLD_NAMESPACE environment variables should also be honoured, and the PR should be refactored to have just one func for finding the namespace to look for pods. Also, if the pods in the *.yaml files are explicitly put in a different namespace [with metadata.namespace], that namespace should also be watched.

@nkubala
Copy link
Contributor

nkubala commented Jan 25, 2019

hey @michaelfig, thanks for the contribution. agree with @r2d4 that this is a fine first step, and thanks for the explanation. looks like this broke some of the sync tests though, can you check out the failures?

@michaelfig
Copy link
Contributor Author

@nkubala, will do (probably next week), likely caused by one of the above known deficiencies.

@michaelfig
Copy link
Contributor Author

This version now honours the .kube/config default namespace, which can be overridden with the -n (--namespace) option to skaffold. There is not yet any work for extracting the namespaces from the configured pod resources.

@r2d4 r2d4 added the kokoro:run runs the kokoro jobs on a PR label Jan 29, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 29, 2019
Copy link
Contributor

@r2d4 r2d4 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 adding this feature!

@michaelfig
Copy link
Contributor Author

michaelfig commented Jan 29, 2019

@r2d4 Failed kokoro, but I can't see the logs (Google corp only).

@r2d4
Copy link
Contributor

r2d4 commented Jan 29, 2019

@michaelfig the logs should be publicly accessible. The page takes a second to load, but you should be able to click through runtimes-common/skaffold/presubmit to see the logs

@michaelfig
Copy link
Contributor Author

michaelfig commented Jan 29, 2019

@r2d4 Thanks, I was able to see the failure (in syncer), and fixed it so that it works in my own cluster. kokoro should run successfully next time.

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Jan 29, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 29, 2019
@michaelfig
Copy link
Contributor Author

@r2d4, @nkubala, @dgageot: Anything else need doing before this can be merged?

Thanks,
Michael.

@nkubala
Copy link
Contributor

nkubala commented Jan 30, 2019

@michaelfig thanks for seeing this one through!

@nkubala nkubala merged commit da8f60e into GoogleContainerTools:master Jan 30, 2019
@michaelfig michaelfig deleted the pod-watch-ns branch January 31, 2019 00:01
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

7 participants