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

Fix port-forwarding not being triggered. #1433

Merged

Conversation

bmcustodio
Copy link
Contributor

@bmcustodio bmcustodio commented Dec 29, 2018

While running skaffold dev to deploy a pod containing some port definitions, I noticed that port-forwardings were not being triggered. Turns out this was happening because...

  • ADDED events are currently ignored by the port forwarder's pod watcher; and
  • The pod I was deploying was already in the Running phase when the watcher is established; and
  • No further events (i.e. of type MODIFIED) were received for that pod (so as to trigger forwarding).

My proposal is that the port forwarder's pod watcher starts taking ADDED events into account.

Signed-off-by: Bruno Miguel Custodio <[email protected]>
@codecov-io
Copy link

codecov-io commented Dec 29, 2018

Codecov Report

Merging #1433 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1433      +/-   ##
==========================================
- Coverage   44.56%   44.53%   -0.03%     
==========================================
  Files         112      112              
  Lines        4593     4596       +3     
==========================================
  Hits         2047     2047              
- Misses       2339     2342       +3     
  Partials      207      207
Impacted Files Coverage Δ
pkg/skaffold/kubernetes/port_forward.go 41.48% <0%> (-1.37%) ⬇️

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 182b762...d2445b1. Read the comment docs.

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.

Thanks - looks like I made a bad assumption

@dgageot dgageot merged commit 60fb2e5 into GoogleContainerTools:master Dec 31, 2018
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.

4 participants