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

fix(parseKeyPairsIntoRecord): allow equals in baggage value #3974 #3975

Merged
merged 6 commits into from
Aug 7, 2023
Merged

fix(parseKeyPairsIntoRecord): allow equals in baggage value #3974 #3975

merged 6 commits into from
Aug 7, 2023

Conversation

krosenk729
Copy link
Contributor

@krosenk729 krosenk729 commented Jul 7, 2023

proposed fix for parseKeyPairsIntoRecord issue 3974

Which problem is this PR solving?

Baggage header values can contain equals as part of their value. parseKeyPairsIntoRecord was treating every = as a delimeter

Fixes #3974

Short description of the changes

This will allow parseKeyPairsIntoRecord('key=something==') to return parsed header values

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • parseKeyPairsIntoRecord('key1=something==')
  • parseKeyPairsIntoRecord('key1=something==,key2=somethingelse')

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@krosenk729 krosenk729 requested a review from a team July 7, 2023 12:44
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 7, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@pichlermarc
Copy link
Member

pichlermarc commented Jul 10, 2023

Hi @krosenk729, thank you for your contribution. It looks like there are still some tests that are failing. Looking a bit into it, it looks like some tests that already existed are now outdated (double equal is given as an invalid example, when it is in fact valid). Feel free to adapt those 🙂

@krosenk729
Copy link
Contributor Author

thank you so much for taking a look @pichlermarc ... I (hopefully) fixed that so the changes are good.

I also see github saying the branch is out of sync with main, and I'm not sure what your convention is with that. just let me know if I need to do a back-merge or anything. thanks

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #3975 (25308b0) into main (3732256) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 25308b0 differs from pull request most recent head 596d4ee. Consider uploading reports for the commit 596d4ee to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3975   +/-   ##
=======================================
  Coverage   92.37%   92.37%           
=======================================
  Files         321      321           
  Lines        9264     9264           
  Branches     1968     1968           
=======================================
  Hits         8558     8558           
  Misses        706      706           
Files Changed Coverage Δ
packages/opentelemetry-core/src/baggage/utils.ts 75.00% <100.00%> (ø)

@pichlermarc
Copy link
Member

Hi @krosenk729, please also add a changelog entry in CHANGELOG.md (you can just follow the same format there), then we can get this merged. 🙂

cc @dyladan I know you've done some work on the baggage spec, would you mind also taking a look at this PR? 🙂

@krosenk729
Copy link
Contributor Author

@pichlermarc thanks! updated!

@krosenk729
Copy link
Contributor Author

hey @pichlermarc anything else I need to do for this one? thanks

@pichlermarc pichlermarc changed the title fix(parseKeyPairsIntoRecord): allow equals in baggage value 3974 fix(parseKeyPairsIntoRecord): allow equals in baggage value #3974 Aug 7, 2023
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

@krosenk729, no nothing to do anymore. thanks! 🙂
Let's get that into the next release. 🙂

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.

parseKeyPairsIntoRecord does not parse if value conatains equals
4 participants