Skip to content

Conversation

Shawnpku
Copy link
Contributor

Add support for Alibaba Cloud CDN (Content Delivery Network) when using OSS storage.

@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 "master" git@github.com:Shawnpku/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.

Signed-off-by: Shawnpku <chen8132@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Didn't do a full review, but I saw this PR, and my eye fell on these 😅

@Shawnpku
Copy link
Contributor Author

Shawnpku commented Feb 11, 2019

@thaJeztah Thank you so much for the review. Already fixed.

@Shawnpku
Copy link
Contributor Author

@thaJeztah Check the changes at your convenient. Thanks.

@thaJeztah
Copy link
Member

Thanks for updating! I'm not a maintainer in this repository, so I'll leave the more in-depth review to the maintainers (I just spotted those small issues 🤗 )

@thaJeztah
Copy link
Member

oh! I see you missed a DCO sign-off on the second commit; could you

  • Squash your commits (no need to keep the second commit separate, as it was only a fix-up of the first one)
  • Use your real name for the sign-off (not your github handle)?

So

Signed-off-by: Shawn Chen <chen8132@gmail.com>

Instead of

Signed-off-by: Shawnpku <chen8132@gmail.com>

Signed-off-by: Shawnpku <chen8132@gmail.com>
@Shawnpku
Copy link
Contributor Author

Please take a look @dmcgowan @caervs @manishtomar. Thanks.

Copy link
Contributor

@caervs caervs left a comment

Choose a reason for hiding this comment

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

Good stuff! Just one question.

Let's ship on green. Let us know if you have any questions about CI.

Shawnpku added 3 commits March 4, 2019 14:53
Signed-off-by: Shawnpku <chen8132@gmail.com>
Signed-off-by: Shawn Chen <chen8132@gmail.com>
Signed-off-by: Shawn Chen <chen8132@gmail.com>
@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #2849 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2849   +/-   ##
=======================================
  Coverage   60.45%   60.45%           
=======================================
  Files         102      102           
  Lines        8001     8001           
=======================================
  Hits         4837     4837           
  Misses       2517     2517           
  Partials      647      647
Flag Coverage Δ
#linux 60.45% <ø> (ø) ⬆️

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 6d62eb1...51bb5ce. Read the comment docs.

@Shawnpku
Copy link
Contributor Author

Shawnpku commented Mar 4, 2019

@caervs I think this is good now.

@caervs caervs requested a review from manishtomar March 5, 2019 00:44
@Shawnpku
Copy link
Contributor Author

@manishtomar would you like to take a review?

Copy link
Contributor

@manishtomar manishtomar left a comment

Choose a reason for hiding this comment

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

Code LGTM. However, please change the package name and put it up for review again.

Signed-off-by: Shawn Chen <chen8132@gmail.com>
@Shawnpku
Copy link
Contributor Author

@manishtomar Thank you for the review. Changes applied.

Signed-off-by: Shawn Chen <chen8132@gmail.com>
@Shawnpku
Copy link
Contributor Author

Shawnpku commented Apr 1, 2019

@dmcgowan @caervs @manishtomar Could someone merge this PR if it is good now. Thanks.

@caervs caervs merged commit 3226863 into distribution:master Apr 17, 2019
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.

5 participants