Restore import path with uppercase characters in custom build ko example #6286
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
Codecov Report
@@ Coverage Diff @@
## main #6286 +/- ##
=======================================
Coverage 70.38% 70.38%
=======================================
Files 501 501
Lines 22755 22755
=======================================
Hits 16015 16015
Misses 5696 5696
Partials 1044 1044
Continue to review full report at Codecov.
|
|
@halvards not sure why kokoro job keeps failing. This is the error. Is it related to go v1.16? |
Thanks for the heads up. Not sure, I'll take a look at this tomorrow. |
|
@halvards this should be fixed by rebasing onto the latest commit from |
I wasn't able to reproduce the issue described in GoogleContainerTools#5492 and GoogleContainerTools#5505, so this change restores the import path with the uppercase characters and the `ko://` prefix in the custom build example. I had to tweak some values to get the integration tests to run, because I don't have access to the `k8s-skaffold` project. Let's see if the build passes. Additional minor changes: - Bump the ko version in the custom build example. - Add the full path to the ko binary in the custom build script, in case `$GOPATH/bin` is not on the `PATH`. Once we move to Go 1.16 for our builds, we can use the `go install` command to install ko in the custom build shell script.
It's difficult to know what's on the `PATH` in different environments. This change to the custom builder example looks for the ko binary in the `GOPATH/bin` directory.
Hypothesis: `tagPolicy: sha256` doesn't behave correctly with images sideloaded into kind snf k3d. Also fix conditional in custom build example shell script to prevent recompiling ko each time.
I was able to reproduce the issue locally and work out a fix. I'd be happy for feedback on whether I've chosen the right place to fix it. When determining which images to sideload (for kind and k3d), we compare `localImages` and `deployerImages`. The former from `skaffold.yaml`, and the latter from Kubernetes manifest files. The image names from Kubernetes manifests are sanitized in `pkg/skaffold/kubernetes/manifests/images.go#L51` (and `L113`) in the call to docker.ParseReference. The same doesn't happen to image names from `skaffold.yaml`. This change sanitizes these image names just for determining whether to sideload the images. In other parts of the code we look up image pipelines from `skaffold.yaml` using the image name, so I was hesitant to change how `localImages` is used (with 'raw' image names). The hypothesis from the previous commit is disproven, so I'm adding back the `sha256` tag policy in the custom builder example. To make the test case easier to identify from the build logs, I renamed the pod in the custom builder example. New hypothesis: Could this be related to the issues some users are reporting with images not being sideloaded when using Helm? E.g., GoogleContainerTools#5159
|
just did a rebase for you. Hopefully this attempt will succeed. |
|
ahh kokoro run didn't happen. Running one now. |
halvards
added a commit
to halvards/skaffold
that referenced
this pull request
Aug 16, 2021
Decide on how ko builder users should specify the location of `package main` for their images. Previous discussion: GoogleContainerTools#6054 (comment) Tracking: GoogleContainerTools#6041 Related: GoogleContainerTools#6054, GoogleContainerTools#6286
halvards
added a commit
to halvards/skaffold
that referenced
this pull request
Aug 16, 2021
Decide on how ko builder users should specify the location of `package main` for their images. Previous discussion: GoogleContainerTools#6054 (comment) Tracking: GoogleContainerTools#6041 Related: GoogleContainerTools#6054, GoogleContainerTools#6286
tejal29
pushed a commit
that referenced
this pull request
Aug 16, 2021
* Refine ko builder behavior for main packages Decide on how ko builder users should specify the location of `package main` for their images. Previous discussion: #6054 (comment) Tracking: #6041 Related: #6054, #6286 * Improve explanation of the Target config field Clarify how a blank value works, and also how users can use a pattern with wildcards.
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.
Fixes: #5505
Related: #5492 #6041
Description
Updated: I was able to reproduce the issue locally and work out a fix. I'd be happy for feedback on whether I've chosen the right place to fix it.
When determining which images to sideload (for kind and k3d), we compare
localImagesanddeployerImages. The former fromskaffold.yaml, and the latter from Kubernetes manifest files. The image names from Kubernetes manifests are sanitized inpkg/skaffold/kubernetes/manifests/images.go#L51(andL113) in the call todocker.ParseReference().The same doesn't happen to image names from
skaffold.yaml. This change sanitizes these image names just for determining whether to sideload the images.In other parts of the code we look up image pipelines from
skaffold.yamlusing the image name, so I was hesitant to change howlocalImagesis used (with 'raw' image names).To make the custom builder test case easier to identify from the build logs, I renamed the pod in the example.
Hypothesis: Could this be related to the issues some users are reporting with images not being sideloaded when using Helm? E.g., #5159
Additional minor changes:
Bump the ko version in the custom build example.
Add the full path to the ko binary in the custom build script, in case
$GOPATH/binis not on thePATH.Once we move to Go 1.16 for our builds, we can use the
go installcommand to install ko in the custom build shell script.