-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Upgrade go-containerregistry third-party library #957
Merged
cvgw
merged 1 commit into
GoogleContainerTools:master
from
antechrestos:fix/scopes_asked_to_remote_registry
Jan 29, 2020
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antechrestos do you have a link to some info or would you mind giving me a quick explanation on why this is the correct fix for the problem? AFAIK this is correct, but I can't really find much info about this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvgw I crawled around the web to find the way to fix this problem. I though that using a simple commit id would be better, yet
go tidyreplaced it with worse version.Aside from that I don't understand why go 1.13 does not allow the minus zero, as minus zero means do nothing.
Truly I am very confused with that: it allows using
github.com/docker/docker v1.14.0-0.20190319215453-e7b5f7dbe98cinrequiresection, but if one of the dependency needs it with the very same version, it cries.I would have preferred something like removing every
-0.fromgo.modfile :Yet it does not find dependencies after that.:
I am opened to any suggestion on this point as I am not an expert on go modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that additional information @antechrestos
I found this documentation which I think is particularly relevant https://golang.org/cmd/go/#hdr-Pseudo_versions
This section caught my eye
I think that explains why
go tidyupdated the replace statements when you used the commit id.I know there was some additional pseudo version validation added in golang 1.13 which probably has something to do with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antechrestos did you use
go mod edit -replacefor these?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvgw
I did used the
go mod edit -replacecommand at first but did not like the result as I think it is less nice.I've just used it and here is the diff, please let me know if you prefer this way:
Shall I group the replace in a single
replacesection?BTW, would you share the output of kaniko tests?
I launched them from
masterbranch and I have random results.First launch on
masterI cleaned images, containers, and gcloud and retried (still on
master):Do you also observe random results on migration tests? 🤔
I also launched on my branch:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antechrestos I don't have a strong preference between
go mod editversion and yours. I would say keep what you have.The following tests failed
Three of them seem to be the same error
error building image: file with no instructionswhich seems to be coming from buildkit https://github.com/moby/buildkit/blob/f53dcc830b03ccc1719fb9472852dcb960131e24/frontend/dockerfile/parser/parser.go#L280But it certainly doesn't look like those dockerfiles are empty https://github.com/GoogleContainerTools/kaniko/blob/master/integration/dockerfiles/Dockerfile_test_add_dest_symlink_dir
Looking at the integration code I don't see any obvious reasons for the failure. Need to investigate more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvgw Thanks for the info. I will take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvgw I think I solved one of the two problems.
The first one is linked to evolution in parser. This method is called by pkg/executor/build.go:resolveOnBuild with an empty array as
config.OnBuild. It used to be accepted by buildkit but no longer. No more Mr nice guy they may have though.I changed the
if config.OnBuild == niltoif config.OnBuild == nil || len(config.OnBuild) == 0in pkg/executor/build.go:resolveOnBuild and it solved tests with Dockerfile_test_replaced_symlinks, Dockerfile_test_add_dest_symlink_dir and Dockerfile_test_hardlink.The second issue is linked to Dockerfile_test_scratch. It has an
ARGnamedimage. Don't know why but, it also has a meta argument (don't know what it is...yet) also namedimageand valued to empty string; this late one overrides the build arg and the image resolution made by pkg/util/image_util.go:RetrieveSourceImage results in empty value. (ERRO[0000] Error while retrieving image from cache: could not parse reference:)I will go deeper for this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it: now in moby BuildEnvs function they override value while before they kept the previous one. I will try to solve it by putting meta args first and then build args (which is logic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌