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

Carry permissions over when excluding files #23

Merged
merged 1 commit into from
Sep 15, 2017

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Sep 13, 2017

When excluding files from compiled zips in the plugin, permissions were lost. JSZip was not detecting or setting them from the old archive but this patch now correctly does it inside applyZipExclude.

@arichiardi arichiardi changed the title Add defaults for zipped files and dirs Add default permissions for zipped files and dirs Sep 13, 2017
@arichiardi arichiardi changed the title Add default permissions for zipped files and dirs Add default permissions for the Lumo zip Sep 13, 2017
(.file archiver f)
(.apply (.-file archiver) archiver (clj->js (seq f)))))
(.file archiver f #js {:mode 644})
(.apply (.-file archiver) archiver
Copy link
Member

Choose a reason for hiding this comment

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

This line is quite hard to follow - if it's correct, can you bind the update expression in a let and use it by name inside .apply?

Copy link
Contributor Author

@arichiardi arichiardi Sep 14, 2017

Choose a reason for hiding this comment

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

sure can do that.

@arichiardi
Copy link
Contributor Author

Hold on, this one apparently does not do what I want, not working recursively. I am digging.

@arichiardi
Copy link
Contributor Author

So this was the problem in #24

@arichiardi arichiardi changed the title Add default permissions for the Lumo zip Carry permissions over when excluding files Sep 14, 2017
@@ -22,7 +22,8 @@
- https://archiverjs.com/docs/Archiver.html#file"
[output-path zip-opts compiler-opts]

(let [archiver (archiver "zip" #js {:zlib {:level 9}})
(let [archiver (archiver "zip" #js {:zlib {:level 9}
:comment (str "Generated by serverless-cljs-plugin on" (.toISOString (js/Date.)))})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mirroring the one in applyZipExclude which is not always called, so here we take care of that.

Copy link
Member

Choose a reason for hiding this comment

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

It's a very minor issue, but perhaps const comment = ... before the first use might be easier to follow than having very slightly different timestamps for each entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will do

When excluding files from compiled zips in the plugin, permissions were
lost. JSZip was not detecting or setting them from the old archive but this
patch now correctly does it inside applyZipExclude.
@arichiardi
Copy link
Contributor Author

So this one should be good now, I addressed your valid point.

@moea moea merged commit 135fae0 into nervous-systems:master Sep 15, 2017
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

2 participants