Revise port-forward behaviour #5554
Conversation
Turns out pflag supports a NoOptDefValue flag to provide a default value when there is no flag value (e.g., `--port-forward`) which means we can support all of our changes into `--port-forward` and avoid a new flag. Split out resource-forwarding and user-defined forwarding.
briandealwis
requested review from
chanseokoh,
loosebazooka and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #5554 +/- ##
==========================================
- Coverage 70.64% 70.63% -0.01%
==========================================
Files 410 411 +1
Lines 15676 15773 +97
==========================================
+ Hits 11074 11142 +68
- Misses 3790 3811 +21
- Partials 812 820 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Changes look good to me except for a couple of suggestions and nits.
I found compat very hard for me to understand. Is it a known shorthand for backward compatibility?
I see you don't have compat in the flag help but except it as a valid arg the Validate() function.
Exposing compat to user is a bit odd if you do plan to.
| func debugPorts(pod *v1.Pod, c v1.Container) []v1.ContainerPort { | ||
| var ports []v1.ContainerPort | ||
|
|
||
| annot, found := pod.ObjectMeta.Annotations[debugging.DebugConfigAnnotation] |
There was a problem hiding this comment.
Not related to this PR, but curious about why are we using pod annotations to store debug configurations?
Is it only for IDEs? DO you know if IDES do use this metadata and if yes is can we instead stick debug metadata in the new proposed metadata API.
There was a problem hiding this comment.
It's there for tooling to consume. Funnily enough, the major consumer is Skaffold itself which monitors for debuggable pods and turns them into DebuggingContainerEvent:
skaffold/pkg/skaffold/kubernetes/debugging/container_manager.go
Lines 92 to 93 in 5bf0323
(what's the new metadata api?)
There was a problem hiding this comment.
Sorry missed responding to this.
re: whats is the new metadata API.
For event API V2, there are some changes suggested by VSC to add additional metadata for driving the proposed new view. Skaffold does have a MetaData or GetState endpoint which we are thinking of revamping.
briandealwis
force-pushed
the
port-forward-refactor
branch
from
738be40 to
f65bbe2
Compare
briandealwis
added
kokoro:force-run
|
Committing: Kokoro is having some issues. |
Fixes #4163
Related: #4832
Merge after: #5548, #5632 (support for SliceValue)
Description
This PR refactors our port-forward behaviour to implement the design in #4832. At its heart:
userfor user-defined port-forwards as expressed in the skaffold.yaml,servicesfor deployed services,debugfor debug ports, andpodsfor all container ports on pods.--port-forward=offdisables all port-forwarding (or any boolean value accepted by Go)devanddebug(vs previously required--port-forward).runanddeploydefault tooffas before.--port-forwardforwardsuser, andserviceslike before. When used withdebug, it will also forward debugging ports too. aka--port-forward=trueor=compat.--port-forward=a,b,callows a combination of classes to includeThis is mostly refactoring work in the port-forwarding implementation:
NewResourceForwarderinto aNewUserDefinedForwarderandNewServicesForwarderdebugis a subset ofpodsso I switched upNewWatchingPodForwarderto take a port filter for filtering the containerPorts.User facing changes (remove if N/A)
Skaffold now enables user-defined port-forwards in
skaffold.yamlby default; use--port-forward=offto disable entirely.Follow-up Work (remove if N/A)
Provide a way to configure this with the global config.