Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

For gzip encoded response, unzip before logging response. #62

Merged
merged 2 commits into from
Jan 19, 2018

Conversation

kishorenc
Copy link
Contributor

This fixes https://github.com/zapier/zapier/issues/14888

The issue is on all gzip encoded responses. Here's a recent Graylog entry with bad response content: https://logs.internal.zapier.com/messages/graylog_1833/c0394913-faaf-11e7-be8a-0e0ca1cbbd4e

I would like some help in testing this locally with an actual CLI app to verify the fix on Graylog (probably on staging).

@kishorenc kishorenc requested a review from xavdid January 16, 2018 11:44
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

Cool! Thanks for doing this.

As far as testing goes, I whipped up a sample app what gets gzipped data from google (and doesn't require auth). It's pushed as App2174CLIAPI and the source is here: https://gist.github.com/xavdid/c2e87a363bbd92a2c11eea147d558323 (our cli code syncing is busted right now). I've confirmed the output from this call is garbled when tests are run locally, so we should be able to test the fix in GL.

Side note, local always just spits out the response, but it could be worthwhile doing the same fix in CLI if it's useful to see the output when testing with --debug. It seems like it hasn't come up as a feature request yet though, so I wouldn't worry about it for now.

Anyway, it'll be tough to test this in staging. During build, we check the version against known platform version (source) and though npm can uses branches, I can't imagine we allow that. Maybe talk @eliangcs into releasing this as a beta, which might trick whatever we use to verify server side that this is allowed.

If that doesn't work, we could push a version of zapier to staging that forgoes the version verification and deploy that way, probably.

@kishorenc
Copy link
Contributor Author

@xavdid Thanks for the review. What I meant by testing on staging is, primarily pointing the LOGGING_ENDPOINT to Graylog staging and just verifying that response content in the logs don't show up as garbled anymore. If you have already verified that, then I think we can merge.

@xavdid
Copy link
Contributor

xavdid commented Jan 18, 2018

Ah, that does sound way easier!

I've got the stuff to test. I guess all I'd need from you is the endpoint to use and potentially a token? create-logger.jsthis line which means nothing from local gets logged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants