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

Bump go-containerregistry to latest #6073

Merged
merged 2 commits into from
Feb 13, 2020

Conversation

sayboras
Copy link
Contributor

fixes #6045

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 12, 2019
@k8s-ci-robot
Copy link
Contributor

Welcome @sayboras!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @sayboras. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 12, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sayboras
To complete the pull request process, please assign ra489
You can assign the PR to them by writing /assign @ra489 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tstromberg
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 12, 2019
@tstromberg
Copy link
Contributor

Does make work for you? I noticed the build failed on Travis:

# github.com/google/go-containerregistry/pkg/v1/daemon
../../../../pkg/mod/github.com/google/go-containerregistry@v0.0.0-20191211193041-0eaa33c3d13c/pkg/v1/daemon/image.go:73:14: undefined: client.NewClientWithOpts
../../../../pkg/mod/github.com/google/go-containerregistry@v0.0.0-20191211193041-0eaa33c3d13c/pkg/v1/daemon/image.go:73:39: undefined: client.FromEnv
../../../../pkg/mod/github.com/google/go-containerregistry@v0.0.0-20191211193041-0eaa33c3d13c/pkg/v1/daemon/write.go:38:14: undefined: client.NewClientWithOpts
../../../../pkg/mod/github.com/google/go-containerregistry@v0.0.0-20191211193041-0eaa33c3d13c/pkg/v1/daemon/write.go:38:39: undefined: client.FromEnv
Makefile:149: recipe for target 'out/minikube' failed
make: *** [out/minikube] Error 2
The command "make" exited with 2.

@sayboras sayboras changed the title Bump go-containerregistry to latest WIP Bump go-containerregistry to latest Dec 13, 2019
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 13, 2019
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 13, 2019
@sayboras sayboras force-pushed the feature/pump-go-containerregistry branch from d3b041d to 05fe671 Compare December 13, 2019 05:29
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 13, 2019
@TravisBuddy
Copy link

Travis tests have failed

Hey @sayboras,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 19448040-1d6a-11ea-a2fb-51cb67500490

@TravisBuddy
Copy link

Travis tests have failed

Hey @sayboras,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 41211f10-1d6a-11ea-a2fb-51cb67500490

@minikube-bot
Copy link
Collaborator

All Times minikube: [ 130.322024 129.399386 126.339407]
All Times Minikube (PR 6073): [ 126.141168 127.327758 129.108766]

Average minikube: 128.686939
Average Minikube (PR 6073): 127.525897

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6073) |
+----------------------+-----------+--------------------+
| minikube v           |  0.279722 |           0.277425 |
| Creating kvm2        | 49.128884 |          48.641624 |
| Preparing Kubernetes | 52.198631 |          50.965780 |
| Pulling images       |  3.067135 |           2.929191 |
| Launching Kubernetes | 21.062848 |          21.732110 |
| Waiting for cluster  |  2.899509 |           2.928888 |
+----------------------+-----------+--------------------+

@sayboras
Copy link
Contributor Author

sayboras commented Dec 13, 2019

@tstromberg The latest version of go-containerregistry requires docker/docker github.com/docker/docker v1.4.2-0.20190924003213-a8608b5b67c7 https://github.com/google/go-containerregistry/blob/master/go.mod#L15. However, this version will break https://github.com/samalba/dockerclient (for test cases only). As this client is no longer updated and maintained (docker-archive/classicswarm#1879). For the time being, I have raised machine-drivers/machine#26 to replace it.

Need your input if there is any alternative approach.

= go mod ================================================================
k8s.io/minikube/pkg/minikube/cluster imports
        github.com/docker/machine/libmachine/provision imports
        github.com/samalba/dockerclient tested by
        github.com/samalba/dockerclient.test imports
        github.com/docker/docker/pkg/jsonlog: module github.com/docker/docker@latest found (v1.13.1, replaced by github.com/docker/docker@v1.4.2-0.20190924003213-a8608b5b67c7), but does not contain package github.com/docker/docker/pkg/jsonlog

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 30, 2019
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 30, 2019
@minikube-bot
Copy link
Collaborator

All Times minikube: [ 122.302872 126.000934 116.147404]
All Times Minikube (PR 6073): [ 125.344900 125.961995 123.398957]

Average minikube: 121.483737
Average Minikube (PR 6073): 124.901951

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6073) |
+----------------------+-----------+--------------------+
| minikube v           |  0.419635 |           0.325190 |
| Creating kvm2        | 47.515834 |          47.363959 |
| Preparing Kubernetes | 48.201943 |          51.351617 |
| Pulling images       |  2.521331 |           3.290899 |
| Launching Kubernetes | 20.043820 |          19.998926 |
| Waiting for cluster  |  2.734920 |           2.575150 |
| Done                 |           |                    |
|                      |  0.046253 |           0.046118 |
+----------------------+-----------+--------------------+

@sayboras
Copy link
Contributor Author

wait for samalba/dockerclient#246 to address the above issue, not sure if the author is still checking PR.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 23, 2020
@minikube-pr-bot
Copy link

All Times minikube: [ 93.560859 96.823319 96.816873]
All Times Minikube (PR 6073): [ 95.165164 94.765806 96.240286]

Average minikube: 95.733684
Average Minikube (PR 6073): 95.390419

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6073) |
+----------------------+-----------+--------------------+
| minikube v           |  0.314336 |           0.290161 |
| Creating kvm2        | 20.438050 |          19.494720 |
| Preparing Kubernetes | 48.540943 |          49.141032 |
| Pulling images       |  3.717667 |           3.840928 |
| Launching Kubernetes | 19.294599 |          19.365360 |
| Waiting for cluster  |  2.881102 |           2.715649 |
+----------------------+-----------+--------------------+

@sayboras
Copy link
Contributor Author

@medyagh need your help on integration test, I tried to run it locally on my macbook, but got another issue :(. I have ubuntu and mac for testing. Appreciate if you can give me how to do it so that feedback is faster.

FYI. I compile minikube binary and manage to provision cluster successfully.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2020
@priyawadhwa
Copy link

Hey @sayboras thank you for opening this PR. I think it's pretty close to being merged, could you please resolve merge conflicts?

Also, what issue did you run into when running integration tests locally? We have some documentation around it which could be helpful.

@medyagh
Copy link
Member

medyagh commented Feb 11, 2020

@sayboras are you still interested to finish this PR, seems like only needs to resolve merge conflicts?

@sayboras sayboras force-pushed the feature/pump-go-containerregistry branch from bedb86e to 77c7500 Compare February 11, 2020 22:34
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 11, 2020
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 12, 2020
@sayboras sayboras changed the title WIP: Bump go-containerregistry to latest Bump go-containerregistry to latest Feb 12, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2020
@@ -334,7 +334,7 @@ func validateCacheCmd(ctx context.Context, t *testing.T, profile string) {
}
t.Run("cache", func(t *testing.T) {
t.Run("add", func(t *testing.T) {
for _, img := range []string{"busybox", "busybox:1.28.4-glibc", "k8s.gcr.io/pause:latest"} {
for _, img := range []string{"busybox:latest", "busybox:1.28.4-glibc", "k8s.gcr.io/pause:latest"} {
Copy link
Contributor Author

@sayboras sayboras Feb 12, 2020

Choose a reason for hiding this comment

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

we need to have latest due to https://github.com/google/go-containerregistry/pull/404/files, which allows to have both tag and digest (i.e the latest tag is not added automatically)

@sayboras sayboras force-pushed the feature/pump-go-containerregistry branch from 88be5b5 to c0130ec Compare February 12, 2020 04:25
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 12, 2020
@minikube-pr-bot
Copy link

All Times minikube: [ 98.257825 92.944797 95.032189]
All Times Minikube (PR 6073): [ 94.055410 89.940828 95.230410]

Average minikube: 95.411604
Average Minikube (PR 6073): 93.075549

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6073) |
+----------------------+-----------+--------------------+
| minikube v           |  0.202333 |           0.197952 |
| Creating kvm2        | 22.235672 |          19.754556 |
| Preparing Kubernetes | 49.815358 |          50.595690 |
| Pulling images       |           |                    |
| Launching Kubernetes | 21.087704 |          20.458624 |
| Waiting for cluster  |  0.054053 |           0.402646 |
+----------------------+-----------+--------------------+

@sayboras
Copy link
Contributor Author

@sayboras are you still interested to finish this PR, seems like only needs to resolve merge conflicts?

Oh sorry, just have some time locked down to finish this. It should be ok now, please let me know if you have any feedback, happy to address.

@sayboras sayboras force-pushed the feature/pump-go-containerregistry branch from c0130ec to 385b94e Compare February 12, 2020 05:17
@medyagh
Copy link
Member

medyagh commented Feb 13, 2020

@sayboras thanks for updating the PR, the Hyperkit Test failures are a lot !

I wonder if this is related to this PR ? https://storage.googleapis.com/minikube-builds/logs/6073/HyperKit_macOS.html

have you tried it on mac yourself? to run the tests

you can run the tests using this command:
if you have a running minikube profile u can test against it this way.

make integration -e TEST_ARGS="-test.run TestFunctional --profile=minikube --cleanup=false"

@sayboras
Copy link
Contributor Author

sayboras commented Feb 13, 2020

@sayboras thanks for updating the PR, the Hyperkit Test failures are a lot !

I wonder if this is related to this PR ? https://storage.googleapis.com/minikube-builds/logs/6073/HyperKit_macOS.html

have you tried it on mac yourself? to run the tests

you can run the tests using this command:
if you have a running minikube profile u can test against it this way.

make integration -e TEST_ARGS="-test.run TestFunctional --profile=minikube --cleanup=false"

Yes, I did with with the cleanup=true

make integration -e TEST_ARGS="-test.run TestFunctional --profile=minikube --cleanup=true" 
go test -v -test.timeout=60m ./test/integration --tags="integration container_image_ostree_stub containers_image_openpgp go_getter_nos3 go_getter_nogcs" -test.run TestFunctional --profile=minikube --cleanup=true
=== RUN   TestFunctional
=== RUN   TestFunctional/serial
=== RUN   TestFunctional/serial/CopySyncFile
=== RUN   TestFunctional/serial/StartWithProxy
....
PASS
ok      k8s.io/minikube/test/integration        168.192s

@medyagh medyagh merged commit b911e8f into kubernetes:master Feb 13, 2020
@medyagh
Copy link
Member

medyagh commented Feb 13, 2020

Thank you very much :)

@sayboras sayboras deleted the feature/pump-go-containerregistry branch February 13, 2020 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update "go-containerregistry" to latest version
9 participants