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

🐛 [RUM-4629] accept null as env/version/service #2781

Merged
merged 1 commit into from
May 24, 2024

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

Some customer pass null as version. Now that we validate that parameter, the initialization fails.

See DataDog/dd-sdk-flutter#616
Regression caused by #2744

Changes

Do not reject null for tags.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner May 24, 2024 13:40
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.59%. Comparing base (05769ac) to head (7c91ae3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2781   +/-   ##
=======================================
  Coverage   93.58%   93.59%           
=======================================
  Files         242      242           
  Lines        7035     7037    +2     
  Branches     1548     1550    +2     
=======================================
+ Hits         6584     6586    +2     
  Misses        451      451           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented May 24, 2024

🚂 Branch Integration: this build is next, merge in < 9m

Commit 7c91ae3f58 will soon be integrated into staging-21.

This build is next! (estimated merge in less than 9m)

Use /to-staging -c to cancel this operation!

dd-mergequeue bot added a commit that referenced this pull request May 24, 2024
Integrated commit sha: 7c91ae3

Co-authored-by: Benoît Zugmeyer <benoit.zugmeyer@datadoghq.com>
@dd-devflow
Copy link
Contributor

dd-devflow bot commented May 24, 2024

🚂 Branch Integration: This commit was successfully integrated

Commit 7c91ae3f58 has been merged into staging-21 in merge commit 61fcd80928.

Check out the triggered pipeline on Gitlab 🦊

@BenoitZugmeyer BenoitZugmeyer merged commit e6db01b into main May 24, 2024
21 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/accept-null-as-version branch May 24, 2024 15:44
@gscandola
Copy link

Hello @BenoitZugmeyer 👋 (😉 ) ,

Introducing the null capability induce a mismatch between the typing InitConfiguration.service and ViewOptions.service (see here).

Is it intended ?

@BenoitZugmeyer
Copy link
Member Author

👋 Hi @gscandola, it's not intended!
We didn't expect null to be passed as version/service. In practice it was the case for certain integrations using init(..), so we fixed it, but we are not aware of such issue for startView(). In any case, let's try to make it uniform #2798

@gscandola
Copy link

Thanks a lot 👍

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

Successfully merging this pull request may close these issues.

5 participants