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

automatically unzip before upload, re-zip any zipped files after Sentry artifact upload #42

Closed
wants to merge 8 commits into from

Conversation

jasonmit
Copy link

@jasonmit jasonmit commented Dec 9, 2017

I am opening this for a discussion, not necessarily to merge (especially the README changes!).

For now I'll continue to maintain this fork.

Fixes #26

fs.writeFileSync(indexPath, index);
},

upload: function(/* context */) {
upload(/* context */) {
Copy link
Author

Choose a reason for hiding this comment

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

This method is all that really changed, focus the review here. Everything else is formatting and will be discarded when I open up the "official" PR if there is interest.

@duizendnegen
Copy link
Collaborator

OK - I know I've maintained for a long time that I wouldn't be supportive of this change, but seems like Sentry isn't moving on this topic... so let's try and get this landed & released.
Can we hide this behind a configuration settings, e.g. unzipBeforeUploading: true?

@jasonmit
Copy link
Author

@duizendnegen sounds good. The current implementation is only re-zip files that have been zipped. I'll adjust it though to also be behind a flag at some point this week.

@jasonmit jasonmit changed the title automatically unzip & re-zip after Sentry upload automatically unzip before upload, re-zip any zipped files after Sentry artifact upload Dec 13, 2017
@jasonmit jasonmit closed this Dec 31, 2017
@kozak
Copy link

kozak commented Mar 23, 2018

@jasonmit Useful stuff! Would you mind sharing if your branch was working fine ?
I am asking because this PR is closed, and this might indicate you have found some problems with the implementation ?
Cheers!

@jasonmit
Copy link
Author

@kozak branch is working great, we use it at Netflix as part of one of our deployments without issue for a few months now. Unfortunately, I can't recall why I closed it now...

@dschmidt
Copy link
Owner

I'm sorry about the situation - to be honest I haven't done any Ember development in quite some time, that makes it really hard to maintain this project.

Just for my understanding: This plugin is currently broken if e-cli-d-gzip is enabled and keep is disabled?

I am worried about the re-zipping part, especially because it doesn't neccessarily use the same zipping algorithm, see e.g. zopfli option of e-cli-d-gzip.
Can't we unzip to a temp dir and upload from there (or directly upload the buffer) to leave the original gzipped files untouched?

If we make it opt in, it shouldn't break anything for anyone - so I'm even okay with the proposed solution here, but please mention the rezipping in the README.
Is it possible to access ember-cli-deploy-gzip's configuration? We could check ember-cli-deploy-gzip's keep and only rezip/unzip if it's disabled and otherwise filter .gz files. Dunno if it's a good idea trying to be too smart - but I assume that would give the best out of the box experience.

Again I'm very sorry for the inconvenience and hope we can solve this issue for good finally.

@jasonmit
Copy link
Author

jasonmit commented Mar 23, 2018

Just for my understanding: This plugin is currently broken if e-cli-d-gzip is enabled and keep is disabled?

I believe it's broken if you're using e-cli-d-gzip. You end up uploading gzipped assets to sentry which sentry does not yet support. Accepts the files but the callstack in the evented messages are unreadable blobs of text.

Can't we unzip to a temp dir and upload from there (or directly upload the buffer) to leave the original gzipped files untouched?

I'm willing to can tackle this.

Again I'm very sorry for the inconvenience

Don't be and thank you for this great addon!

@dschmidt
Copy link
Owner

Awesome!

@kozak
Copy link

kozak commented Mar 24, 2018

Thank you both. @dschmidt is correct in that enabling: keep: true for the gzip plugin helps alleviate the issue. We ended up removing the gzip plugin and enabling gzipping on cloudfront.

@dschmidt
Copy link
Owner

Glad you found a solution that works for you!

@simonihmig
Copy link
Contributor

Cross posting #52 here as another attempt to fix the issue. Feedback appreciated, maybe I'm missing something obvious...

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