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

Normalize capitalization for types while port forwarding #3803

Merged

Conversation

BlenderDude
Copy link
Contributor

Description
Currently, if --port-forward is used, skaffold will create automated port forwards for resources that are not defined in the portForward field. This works great, but sometimes there are issues with capitalization. For example, I was recently working on a project where the type of the service resource was understood to be service by skaffold, but in the portForward section of the skaffold.yaml, I had set the Type to Service. These were interpreted as two different port forwarding configurations and leads to unintentional duplication on the same resource.

My solution to this was to normalize the capitalization when calculating the key for the portForwardEntry. This makes it such that resources of the same type, but different capitalization, will be treated the same. Ex. service/my-api-service and Service/my-api-service will be treated the same.

A unit test has been added to support this.

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #3803 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
...ffold/kubernetes/portforward/port_forward_entry.go 100% <100%> (ø) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 68.29% <0%> (+2.43%) ⬆️

@dgageot dgageot self-assigned this Mar 10, 2020
@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Mar 10, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 10, 2020
@dgageot dgageot merged commit e217013 into GoogleContainerTools:master Mar 10, 2020
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.

None yet

4 participants