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

Create port map on container config if empty #6621

Merged
merged 1 commit into from
Sep 22, 2021

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Sep 21, 2021

This was causing a nil pointer exception when the container didn't have previously defined ports in its config.

This was not caught by tests because these tests were actually silently broken - a debug level log message was being emitted saying these port forward resources were being skipped because they were not of resource type container. Upon adding this to the tests cases, this test failed as expected.

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to remove this emtpy deploy_test.go though

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty file.

@nkubala nkubala marked this pull request as ready for review September 21, 2021 21:04
@nkubala nkubala requested a review from a team as a code owner September 21, 2021 21:04
@nkubala nkubala requested a review from gsquared94 September 21, 2021 21:04
@nkubala nkubala changed the title [WIP] Create port map on container config if empty Create port map on container config if empty Sep 21, 2021
@nkubala
Copy link
Contributor Author

nkubala commented Sep 21, 2021

@briandealwis @tejal29 I had intentionally left that blank file here as a reminder that test cases needed to be added (so CI would fail until they were), but I realized that there were existing (broken) test cases for this. they're now fixed and this should be ready for a look

@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #6621 (06d02e7) into main (290280e) will decrease coverage by 0.34%.
The diff coverage is 79.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6621      +/-   ##
==========================================
- Coverage   70.48%   70.13%   -0.35%     
==========================================
  Files         515      520       +5     
  Lines       23150    23614     +464     
==========================================
+ Hits        16317    16562     +245     
- Misses       5776     5973     +197     
- Partials     1057     1079      +22     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 89.00% <0.00%> (-1.82%) ⬇️
cmd/skaffold/skaffold.go 0.00% <ø> (ø)
pkg/diag/recommender/container_errors.go 0.00% <0.00%> (ø)
pkg/diag/validator/pod.go 1.32% <0.00%> (ø)
pkg/skaffold/build/buildpacks/logger.go 0.00% <ø> (ø)
pkg/skaffold/build/cluster/logs.go 0.00% <ø> (-16.67%) ⬇️
pkg/skaffold/config/options.go 100.00% <ø> (ø)
pkg/skaffold/build/jib/errors.go 48.93% <50.00%> (ø)
cmd/skaffold/app/cmd/cmd.go 70.32% <66.66%> (-0.73%) ⬇️
pkg/diag/validator/resource.go 47.05% <66.66%> (ø)
... and 74 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 d3e1279...06d02e7. Read the comment docs.

@nkubala nkubala merged commit f102665 into GoogleContainerTools:main Sep 22, 2021
@nkubala nkubala deleted the docker-port-fix branch September 22, 2021 20:11
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.

3 participants