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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix header misreporting to engine #1689

Merged
merged 2 commits into from Sep 18, 2018
Merged

Fix header misreporting to engine #1689

merged 2 commits into from Sep 18, 2018

Conversation

JakeDawkins
Copy link
Contributor

@JakeDawkins JakeDawkins commented Sep 18, 2018

Issue

When using the privateHeaders option in the constructor, not all expected headers were being reported to Engine.

This was caused because the loop iterating over the headers had a break where it should have had a continue, causing the loop to be cut short.

Note: This bug didn't cause any private headers to get reported, so no worries there 馃槃

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

Copy link
Contributor

@evans evans left a comment

Choose a reason for hiding this comment

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

@JakeDawkins Thank you for the PR! Not sure why we're getting package-lock differences. Are you running npm 6?

Also can you add this PR to the changelog?

@JakeDawkins
Copy link
Contributor Author

@evans done! I wasn't on npm 6 馃檲 There are still a couple diffs, all changing http urls to https. Not sure what's going on there, but significantly fewer than before.

@evans evans merged commit f3df370 into master Sep 18, 2018
@evans evans deleted the private-headers branch September 18, 2018 20:29
@evans
Copy link
Contributor

evans commented Sep 18, 2018

@JakeDawkins Thank you 馃尞

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

Successfully merging this pull request may close these issues.

None yet

2 participants