portforward: deprecate implicit container port behavior #5735
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tilt supports a bunch of variations when defining a port forward in a Tiltfile. It's possible to specify a single value (a local port), and in this case, Tilt will look for a matching exposed port to use. If none is found, it'll use the first exposed port. This behavior is confusing and doesn't match existing functionality in e.g. `kubectl`. Allowing a single port that will be used identically for local and container is reasonable short-hand, but picking an implicit port to map to adds extra complexity for questionable value. In this latter case, a warning is now emitted along with the mapping and instructions for the user to update their `Tiltfile` to make it explicit before it breaks in a future release of Tilt. To fully deprecate this, we can change the logic to always default the container port to local port. However, as this will change the behavior in the aforementioned case, it's a breaking change, so let's provide a heads up first :) Fixes #5728.
nicksieger
approved these changes
Apr 26, 2022
nicks
reviewed
Apr 26, 2022
| @@ -116,9 +121,12 @@ func (r *Reconciler) toDesiredPortForward(kd *v1alpha1.KubernetesDiscovery) (*v1 | |||
| // | |||
| // TODO(nick): This is old legacy behavior, and I'm not totally sure it even | |||
| // makes sense. I wonder if we should just insist that ContainerPort is populated. | |||
| func populateContainerPorts(pf *v1alpha1.PortForward, pod *v1alpha1.Pod) { | |||
| func populateContainerPorts(pft *v1alpha1.PortForwardTemplateSpec, pod *v1alpha1.Pod) []v1alpha1.Forward { | |||
There was a problem hiding this comment.
do you expect that we will still want to print this warning after we change the default? is there any reason not to change the default now?
There was a problem hiding this comment.
Good question, I was taking a fairly cautious approach here and adding the warning for now before changing the default. At that point, I could see an argument for keeping it to let users know that Tilt is now picking a different port than it would have in the past in this case, but that also might be overkill, e.g. when we remove deprecated functionality, we don't generally leave in a warning if someone tries to use it, we just error out.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tilt supports a bunch of variations when defining a port forward in
a Tiltfile.
It's possible to specify a single value (a local port), and in this
case, Tilt will look for a matching exposed port to use. If none
is found, it'll use the first exposed port.
This behavior is confusing and doesn't match existing functionality
in e.g.
kubectl.Allowing a single port that will be used identically for local and
container is reasonable short-hand, but picking an implicit port
to map to adds extra complexity for questionable value.
In this latter case, a warning is now emitted along with the mapping
and instructions for the user to update their
Tiltfileto make itexplicit before it breaks in a future release of Tilt.
To fully deprecate this, we can change the logic to always default
the container port to local port. However, as this will change the
behavior in the aforementioned case, it's a breaking change, so
let's provide a heads up first :)
Fixes #5728.