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.

Sign up for GitHub

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

Jump to bottom

fix: return 404 on removePackage #289

Merged
merged 2 commits into from
Dec 27, 2019
Merged

fix: return 404 on removePackage #289

merged 2 commits into from
Dec 27, 2019

Conversation

favoyang
Copy link
Contributor

@favoyang favoyang commented Nov 29, 2019 •
edited
Loading

Type: bug
Scope: aws-s3-storage

The following has been addressed in the PR:

Description:
Fixed verdaccio/verdaccio/issues/1435, unpublish -f failed with s3 storage.

When unpublish a package, verdaccio will remove the package.json and all tarballs, then try to remove (source code use word 'unlink') the directory. It is okay in local storage, but not for s3. S3 does not have the directory concept, when remove all objects under a path, the path is gone as well. So try to remove the directory will returns 404 error. Just ignore the 404 error is fine, and will fix the npm client error.

@favoyang favoyang mentioned this pull request Nov 29, 2019
Closed
@codecov
Copy link

codecov bot commented Nov 29, 2019 •
edited
Loading

Codecov Report

Merging #289 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #289     +/-   ##
=========================================
- Coverage   65.42%   65.33%   -0.1%     
=========================================
  Files          26       26             
  Lines        1400     1402      +2     
  Branches      204      202      -2     
=========================================
  Hits          916      916             
- Misses        481      484      +3     
+ Partials        3        2      -1
Flag Coverage Δ
#core 88.18% <ø> (ø) ⬆️
#plugins 58.31% <0%> (-0.1%) ⬇️
Impacted Files Coverage Δ
plugins/aws-s3-storage/src/s3PackageManager.ts 0.64% <0%> (-0.01%) ⬇️
plugins/memory/src/memory-handler.ts 87.17% <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 e269378...d5d94a8. Read the comment docs.

@juanpicado
Copy link
Member

I'll run some test before merge this, I'll work on this.

@favoyang
Copy link
Contributor Author

@juanpicado

minio is a tool to simulate s3 service on local.

Script to start minio server

MINIO_REGION=us-east-1 MINIO_ACCESS_KEY=admin MINIO_SECRET_KEY=password minio server ~/Documents/projects/minio

Verdaccio config section

store:
  aws-s3-storage:
    bucket: openupm # a folder under minio path
    region: us-east-1
    endpoint: http://127.0.0.1:9000
    accessKeyId: admin
    secretAccessKey: password
    s3ForcePathStyle: true
    keyPrefix: 'verdaccio/'

@juanpicado
Copy link
Member

Thanks that is really helpful 🙏

@DanielRuf
Copy link
Contributor

Please describe in the commit message and PR title what it does.

@favoyang favoyang changed the title fix: verdaccio/verdaccio/issues/1435 fix: npm unpublish -f failed with s3 storage (verdaccio/verdaccio/issues/1435) Dec 24, 2019
juanpicado
juanpicado approved these changes Dec 27, 2019
View reviewed changes
Copy link
Member

@juanpicado juanpicado left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think is ok, we have no test running here, so, not 100% confident. But worth to re-visit this. I'd try to enable them again and see what other changes are need it.

Thanks for this.

@juanpicado juanpicado changed the title fix: npm unpublish -f failed with s3 storage (verdaccio/verdaccio/issues/1435) fix: return 404 on removePackage Dec 27, 2019
@juanpicado juanpicado merged commit 7a130ca into verdaccio:master Dec 27, 2019
@juanpicado
Copy link
Member

Try on verdaccio-aws-s3-storage@8.5.3

@juanpicado juanpicado mentioned this pull request Dec 27, 2019
Merged
@liarco
Copy link

liarco commented Dec 27, 2019

The 404 error is now fixed in my setup (Docker + Digital Oceans's spaces). Thank you everyone!

@favoyang favoyang mentioned this pull request Jan 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanpicado juanpicado juanpicado approved these changes

Assignees
No one assigned
Labels
code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"publish --force" and "unpublish VERSION" seem to be broken
4 participants
@favoyang @juanpicado @DanielRuf @liarco