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

rename google.golang.org/cloud to cloud.google.com/go #2524

Closed
wants to merge 2 commits into from

Conversation

gracenoah
Copy link

This import was causing issues with importing distributing using dep. There was no good way to make dep understand that the package was moved and renamed. I'm not sure exactly why, but this fixes it.

Signed-off-by: Grace Noah

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "fix-deps" git@github.com:gracenoah/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@gracenoah
Copy link
Author

gracenoah commented Feb 14, 2018

Interesting...

CI is failing with

vendor/cloud.google.com/go/cloud.go:23:2: use of internal package not allowed
vendor/cloud.google.com/go/option.go:21:2: use of internal package not allowed

Looks like this can't be done unless we also upgrade the package. Any objections to trying a later version? If not, I'll try to find one that works...

@dmcgowan
Copy link
Collaborator

The package has // import "google.golang.org/cloud" which means it is under the correct name. If dep cannot handle the import statements changing correctly then that is a bug in dep. As for updating, we will need a maintainer or contributor to this driver to verify the upgrade.

Also please provide a valid email address in the DCO sign off, we cannot accept contributions if the information is obviously incorrect.

@codecov
Copy link

codecov bot commented Feb 14, 2018

Codecov Report

Merging #2524 into master will decrease coverage by 7.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2524      +/-   ##
==========================================
- Coverage   60.83%   53.48%   -7.36%     
==========================================
  Files         129      126       -3     
  Lines       11864    11327     -537     
==========================================
- Hits         7218     6058    -1160     
- Misses       3743     4506     +763     
+ Partials      903      763     -140
Impacted Files Coverage Δ
registry/storage/driver/oss/oss.go 0.56% <0%> (-56.91%) ⬇️
registry/storage/driver/s3-goamz/s3.go 0.5% <0%> (-51.14%) ⬇️
registry/storage/driver/s3-aws/s3.go 4.15% <0%> (-50.98%) ⬇️
registry/client/transport/transport.go 69.69% <0%> (-9.1%) ⬇️
registry.go
context/context.go
errors.go
vendor/github.com/gorilla/context/context.go 43.75% <0%> (ø)

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 6664ec7...c58a68d. Read the comment docs.

@gracenoah
Copy link
Author

Yes, this is most likely a bug in dep. There's a big discussion about similar dep issues here: golang/dep#860
I'll update the commits with an email address and try to make the upgrade work.

Signed-off-by: Grace Noah <gracenoahgh@gmail.com>
Signed-off-by: Grace Noah <gracenoahgh@gmail.com>
@gracenoah
Copy link
Author

Looks like there's no way to upgrade the library without making changes to the GCS driver. This might just not be an appropriate solution to the vendoring problem. It's sad that the GCS driver is so out of date though :( Let me know if my assessment is correct and feel free to close if that's the case.

@dmp42
Copy link
Contributor

dmp42 commented Mar 21, 2018

Yes, the driver must be updated to match with the latest lib version.

Closing this specific PR.

@dmp42 dmp42 closed this Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants