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

feat(aws-s3-storage): set ACL of tarball files #249

Merged
merged 22 commits into from
May 2, 2021

Conversation

favoyang
Copy link
Contributor

@favoyang favoyang commented Nov 15, 2019

Type: feature
Scope: plugins/aws-s3-storage

The following has been addressed in the PR:

Description:

The supportive PR introduces S3Config.tarballACL to specify ACL (access-control list) of tarball files.

  • set to 'private' by default, same as the current behavior.
  • when set to 'public-read', it will grant tarball files anonymous read permission, which is necessary to enable S3 integrated CDN service like Amazon CloudFront.

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #249 (e417ec0) into 9.x (1e7aeec) will decrease coverage by 14.33%.
The diff coverage is 83.33%.

❗ Current head e417ec0 differs from pull request most recent head 103fae2. Consider uploading reports for the commit 103fae2 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##              9.x     #249       +/-   ##
===========================================
- Coverage   84.74%   70.41%   -14.34%     
===========================================
  Files           1       28       +27     
  Lines          59     1423     +1364     
  Branches       13      205      +192     
===========================================
+ Hits           50     1002      +952     
- Misses          6      419      +413     
+ Partials        3        2        -1     
Flag Coverage Δ
core 88.18% <ø> (?)
plugins 63.13% <83.33%> (-21.62%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
plugins/aws-s3-storage/src/s3PackageManager.ts 43.63% <83.33%> (ø)
core/file-locking/src/index.ts 92.64% <0.00%> (ø)
core/readme/src/index.ts 100.00% <0.00%> (ø)
plugins/htpasswd/src/crypt3.ts 100.00% <0.00%> (ø)
plugins/local-storage/src/local-database.ts 85.62% <0.00%> (ø)
plugins/aws-s3-storage/src/index.ts 0.00% <0.00%> (ø)
plugins/google-cloud/src/data-storage.ts 78.65% <0.00%> (ø)
plugins/active-directory/src/active-directory.ts 100.00% <0.00%> (ø)
plugins/htpasswd/src/utils.ts 98.50% <0.00%> (ø)
plugins/memory/test/partials/pkg.ts 100.00% <0.00%> (ø)
... and 19 more

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 76ac4ba...103fae2. Read the comment docs.

@favoyang favoyang changed the title Added package ACL support. Added tarball S3 ACL support. Nov 18, 2019
@favoyang favoyang changed the title Added tarball S3 ACL support. Use S3 Built-in CDN/Edge to Serve Tarball Files Nov 23, 2019
@favoyang
Copy link
Contributor Author

Based on the reconsideration of verdaccio/verdaccio#1580 (comment), this PR shall be updated to only keep the logic of ACL, and remove the logic of tarballEdgeUrl.

@favoyang favoyang changed the title Use S3 Built-in CDN/Edge to Serve Tarball Files feat(aws-s3-storage): set ACL of tarball files Jan 27, 2020
@favoyang
Copy link
Contributor Author

@juanpicado the PR #249 has been simplified to only include the feature of setting ACL of tarball files. Though it is a supportive PR for #250, but it offering an independent feature, I think it can be reviewed and merged individually.

Please reconsider to remove the do-not-merge label. To review this, I suggest you read the updated description of this PR, #250.

@favoyang favoyang requested a review from juanpicado January 27, 2020 13:34
@juanpicado juanpicado changed the base branch from master to 9.x May 2, 2020 06:03
@juanpicado
Copy link
Member

@favoyang sorry the dealy. I'm ready to merge this. Could you rebase form master and run pnpm changestet?
https://github.com/verdaccio/monorepo/blob/9.x/CONTRIBUTING.md#adding-a-changeset

juanpicado
juanpicado previously approved these changes May 1, 2021
Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@juanpicado juanpicado added the enhancement New feature or request label May 1, 2021
@favoyang favoyang changed the base branch from 9.x to master May 1, 2021 16:32
@favoyang favoyang dismissed juanpicado’s stale review May 1, 2021 16:32

The base branch was changed.

@favoyang favoyang changed the base branch from master to 9.x May 1, 2021 16:57
@favoyang
Copy link
Contributor Author

favoyang commented May 1, 2021

@juanpicado, so I should migrate the PR to verdaccio/verdaccio@master then re-submit a new PR?

@juanpicado
Copy link
Member

No for now, just thinking to merge on 5.x under a flag and let see how it goes. I want to change the plugins architecture (help ls welcome) in master is to early to put this feature there.

@favoyang
Copy link
Contributor Author

favoyang commented May 1, 2021

Sorry that I’m still confused. The 5.x branch on verdaccio seems has no packages folder, and the master branch on monorepo is basically empty. So what should I do next to “rebase from master and run pnpm”?

@juanpicado
Copy link
Member

juanpicado commented May 1, 2021

Sorry that I’m still confused. The 5.x branch on verdaccio seems has no packages folder, and the master branch on monorepo is basically empty. So what should I do next to “rebase from master and run pnpm”?

No problem, sorry I was in a hurry when I wrote.

This PR would be great if you

  • git rebase origin/9.x (it should not have any conflict).
  • Run pnpm install
  • Run pnpm changeset and follow the CONTRIBUTION guidelines to create the changeset. (This will create a markdown file on .changeset folder which you can add enough information which will be part of the CHANGELOG`.

Once it is done, I will merge, the GitHub Action will detect your changeset and create a PR which I'll merge and then the new version will be published in npmjs.

If you like you can also copy this change to https://github.com/verdaccio/verdaccio/tree/master/packages/plugins/aws-storage but this is optional.

@favoyang
Copy link
Contributor Author

favoyang commented May 2, 2021

git rebase origin/9.x (it should not have any conflict).

Not sure for what reasons run rebase 9.x results in conflicts. So I run merge 9.x instead. You should be able to merge this PR with the squash option to get a clean changeset.

Run pnpm install

Done

$ pnpm --version
6.0.1
$ pnpm install
Scope: all 14 workspace projects
Already up-to-date
WARN  Failed to find "/fsevents/1.2.13" in lockfile during hoisting. Next aliases will not be hoisted: fsevents

Run pnpm changeset and follow the CONTRIBUTION guidelines to create the changeset. (This will create a markdown file on .changeset folder which you can add enough information which will be part of the CHANGELOG`.

 $ pnpm changeset

> @verdaccio/[email protected] changeset /home/favo/projects/verdaccio-test/monorepo
> changeset

🦋  Which packages would you like to include? … verdaccio-aws-s3-storage
  ✔ verdaccio-aws-s3-storage

Type/select the verdaccio-aws-s3-storage package, then press enter results in error:

🦋  Which packages would you like to include? · No items were selected
🦋  error You must select at least one package to release
🦋  error (You most likely hit enter instead of space!)

@juanpicado juanpicado merged commit 296a141 into verdaccio:9.x May 2, 2021
@favoyang favoyang deleted the cdn branch May 4, 2021 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants