-
Notifications
You must be signed in to change notification settings - Fork 83
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
archive: Add absolute path mode #141
Conversation
@hacktron95 Thanks for the contribution. We'll have a look at it as soon as we can. |
Hey @hacktron95, sorry I think I dropped the ball on this one. I'll try to spare some time for reviewing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks good to me. Only a few nit comments.
Co-authored-by: Kemal Akkoyun <[email protected]>
hey @kakkoyun thanks a lot for your review, I'll resolve all the suggested changes during the weekend 👍 |
hey @kakkoyun, hope you are doing well!, |
@hacktron95 Hey, I have missed this. I'll take a look at it. Thanks for your patience! |
hello @kakkoyun. Just want to know if this PR will be still considered to be merged. Facing same issue. Thanks! |
It's still considered. Reviews are welcome. It's been a while since the last time I have touched the project. I'll do my best to merge it, no promises. As I told additional reviews and tests would be helpful. |
bump this PR |
I think that use-cases where the paths to cache are outside of the workspace directory are quite common, e.g. when wanting to save the cache of some package managers. I am personally affected by this when trying to save the directory of the Composer cache. Is there any plan to bring this PR home anytime soon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing @ste93cry - this is preventing me from proper caching for a large number of repositories utilizing composer and npm (Node). Any chance of seeing this merged any time soon? For what it's worth, I pulled the author's repo and built it locally for testing with |
I guess we need help from @meltwater-foundation to proceed with this |
@hacktron95 Do you mind pulling the latest master into your working branch and resolving conflicts? After this and passing tests I'm happy to review and merge. |
pinging @hacktron95 - can you review @bdebyl's comment from Jul 19? |
@hacktron95 @mattzuba I can resolve the merge conflicts if either of you are unavailable to. That's all that's left to get this merged and ready for the v1.4.0 release |
I have forked this and the other repos, rebased the commits and done some testing and I'm happy to submit a new PR if you'd like. |
Just came across this thread and really happy to see that the absolute paths are now supported. Are there any examples on how to use it? I'm trying to understand how Drone will handle different Docker images e.g. the plugin and then the Go image that I'm using in my pipeline, as I want to cache Edit: Or is there better ways to do this with vendoring? Would really appreciate some help using this plugin! |
ALL CREDITS goes to @imranismail
Fixes #130
#106 comment
Description
add absolute path mode to
tar.gz
file by checking for any path prefixed with/
add test in
tar_test.gz
for the absolute path mode.since
gzip.go
usestar.gz
underneath it, there was no changed made togzip.go
added test in
gzip_test.go
for the absolute path mode.Read the CONTRIBUTING document.
Read the CODE OF CONDUCT document.
Add tests to cover changes.
Ensure your code follows the code style of this project.
Ensure CI and all other PR checks are green OR
Add your changes to
Unreleased
section of CHANGELOG.Improve and update the README (if necessary).
Ensure documentation is up-to-date. The same file will be updated in plugin index when your PR is accepted, so it will be available for end-users at http://plugins.drone.io.