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.

Sign up for GitHub

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

Jump to bottom

Include runtime dependencies for taggers in gcr.io/k8s-skaffold/skaffold #1987

Merged
merged 5 commits into from
Apr 23, 2019
Merged

Include runtime dependencies for taggers in gcr.io/k8s-skaffold/skaffold #1987

merged 5 commits into from
Apr 23, 2019

Conversation

corneliusweig
Copy link
Contributor

@corneliusweig corneliusweig commented Apr 18, 2019 •
edited
Loading

Also extend integration tests to assert on the expected image tags.
In addition, remove helm-${HELM_VERSION}-linux-amd64.tar.gz from the skaffold image.

Fixes #1979

@corneliusweig corneliusweig changed the title Include runtime dependencies for taggers in gcr.io/k8s-skaffold/skaffold WIP Include runtime dependencies for taggers in gcr.io/k8s-skaffold/skaffold Apr 18, 2019
@corneliusweig corneliusweig changed the title WIP Include runtime dependencies for taggers in gcr.io/k8s-skaffold/skaffold Include runtime dependencies for taggers in gcr.io/k8s-skaffold/skaffold Apr 18, 2019
@codecov-io
Copy link

codecov-io commented Apr 19, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1987      +/-   ##
==========================================
+ Coverage   55.48%   55.49%   +0.01%     
==========================================
  Files         173      173              
  Lines        7510     7512       +2     
==========================================
+ Hits         4167     4169       +2     
  Misses       2952     2952              
  Partials      391      391
Impacted Files Coverage Δ
pkg/skaffold/build/tag/date_time.go 47.36% <100%> (ø) ⬆️
cmd/skaffold/app/cmd/cmd.go 75% <0%> (+0.53%) ⬆️

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 37baddd...083587d. Read the comment docs.

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Apr 19, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 19, 2019
@balopat
Copy link
Contributor

balopat commented Apr 20, 2019

can you please rebase this to master? the kokoro failure is fixed now there. Thank you!

balopat
balopat approved these changes Apr 20, 2019
View reviewed changes
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM

balopat
balopat reviewed Apr 20, 2019
View reviewed changes
deploy/skaffold/Dockerfile Outdated
@@ -14,8 +14,12 @@

FROM gcr.io/gcp-runtimes/ubuntu_16_0_4 as runtime_deps

# needed for dateTime tagger
COPY --from=golang:1.11 /usr/local/go/lib/time/zoneinfo.zip /usr/local/go/lib/time/zoneinfo.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm...this smells a little bit for me...and not just me: golang/go#21881 (comment) - maybe we should try 4d63.com/tz ?

Copy link
Contributor Author

@corneliusweig corneliusweig Apr 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, absolutely! With 4d63.com/tz the dateTime tagger will also work on windows :)

@corneliusweig
Include necessary dependencies for taggers
7e2ab7b
@corneliusweig
Check that the taggers produce an image with the expected tag
1833dd3 
Signed-off-by: Cornelius Weig <[email protected]>
@corneliusweig
Avoid recursive copy of test files
14ee49f 
For the git tagger test, set up a dummy repo inside the testdata folder.

Signed-off-by: Cornelius Weig <[email protected]>
@corneliusweig
Bundle timezone data with the skaffold binary
19a5795 
This ensures that te dateTime tagger works on windows when go is not installed.

Signed-off-by: Cornelius Weig <[email protected]>
@corneliusweig
Copy link
Contributor Author

corneliusweig commented Apr 20, 2019 •
edited
Loading

@balopat PTAL. The first three commits were rebased as requested, the last one is new and adds 4d63.com/tz.

@priyawadhwa priyawadhwa added kokoro:run runs the kokoro jobs on a PR and removed needs-rebase labels Apr 22, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 22, 2019
@dgageot
Copy link
Contributor

dgageot commented Apr 23, 2019

@corneliusweig kokoro tests are failing:

--- FAIL: TestBuild (6.96s)
    --- PASS: TestBuild/docker_build (6.60s)
    --- FAIL: TestBuild/git_tagger (0.01s)
        build_test.go:160: exit status 128

@corneliusweig
Fix integration test setup for git commit tagger
083587d 
Git commits need a configured author.

Signed-off-by: Cornelius Weig <[email protected]>
@corneliusweig
Copy link
Contributor Author

...that was hard to find 😓

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Apr 23, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 23, 2019
@balopat balopat merged commit a756680 into GoogleContainerTools:master Apr 23, 2019
@corneliusweig corneliusweig deleted the skaffold-image branch April 23, 2019 22:09
@dbolkensteyn dbolkensteyn mentioned this pull request May 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balopat balopat balopat approved these changes

@dgageot dgageot dgageot approved these changes

@nkubala nkubala Awaiting requested review from nkubala

@priyawadhwa priyawadhwa Awaiting requested review from priyawadhwa

@r2d4 r2d4 Awaiting requested review from r2d4

@tejal29 tejal29 Awaiting requested review from tejal29

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

skaffold docker doesn't tag correctly
7 participants
@corneliusweig @codecov-io @balopat @dgageot @googlebot @priyawadhwa @kokoro-team