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

Add note about Datadog.configure requiring block in 1.x to Upgrade Guide #1970

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Apr 7, 2022

Spotted this change when working with @lloeki on validating 1.0.0.beta2.

@ivoanjo ivoanjo requested a review from a team April 7, 2022 14:14
Copy link
Contributor

@lloeki lloeki left a comment

Choose a reason for hiding this comment

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

LGTM

@ivoanjo
Copy link
Member Author

ivoanjo commented Apr 7, 2022

@delner any concerns about this change?

Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

I don't really see this particular change as constructive or useful. It calls out a weird usage that was employed as a hack as if it's meant to be used this way.

I wouldn't recommend this. If the hack was predicated by some flaw, it would be better to address that flaw.

@ivoanjo
Copy link
Member Author

ivoanjo commented Apr 8, 2022

@delner Since this is something that changed (and even bit us!) it seems nice to have it documented as a change in the "Breaking Changes" section -- it is an actual breaking change in the public API.

But I do agree that by itself, calling Datadog.configure shouldn't be useful or needed. So what're your thoughts on instead of suggesting:

Use Datadog.configure { } instead.

we suggest:

Remove usages of Datadog.configure without a block.

…Guide

Spotted this change when working with @lloeki on validating
1.0.0.beta2.
@ivoanjo ivoanjo force-pushed the ivoanjo/add-note-about-configure-without-a-block branch from 82b0347 to ce9017b Compare April 14, 2022 10:41
@ivoanjo
Copy link
Member Author

ivoanjo commented Apr 14, 2022

Updated PR with the advice to remove such uses.

@codecov-commenter
Copy link

Codecov Report

Merging #1970 (ce9017b) into master (60ecc8a) will increase coverage by 0.15%.
The diff coverage is 93.96%.

@@            Coverage Diff             @@
##           master    #1970      +/-   ##
==========================================
+ Coverage   97.53%   97.69%   +0.15%     
==========================================
  Files         998     1001       +3     
  Lines       49006    50453    +1447     
==========================================
+ Hits        47800    49291    +1491     
+ Misses       1206     1162      -44     
Impacted Files Coverage Δ
.../datadog/appsec/contrib/rack/request_middleware.rb 34.48% <25.00%> (+10.95%) ⬆️
lib/datadog/appsec/processor.rb 86.20% <86.20%> (ø)
spec/datadog/core/error_spec.rb 94.02% <90.00%> (-0.38%) ⬇️
lib/datadog/ci/contrib/rspec/example.rb 96.96% <100.00%> (+0.41%) ⬆️
spec/datadog/appsec/processor_spec.rb 100.00% <100.00%> (ø)
...c/datadog/ci/contrib/rspec/instrumentation_spec.rb 100.00% <100.00%> (ø)
...c/datadog/ci/contrib/rspec/some_shared_examples.rb 100.00% <100.00%> (ø)
spec/datadog/profiling/ext/forking_spec.rb 99.39% <0.00%> (-0.61%) ⬇️
lib/datadog/ci/test.rb 100.00% <0.00%> (ø)
lib/datadog/ci/flush.rb 100.00% <0.00%> (ø)
... and 214 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

I've used this pattern before, specially when trying to run the host application test with ddtrace enabled.

@ivoanjo
Copy link
Member Author

ivoanjo commented Apr 25, 2022

I spoke with David a week back and he was happy with the alternative advice; merging this in.

@ivoanjo ivoanjo merged commit 6862961 into master Apr 25, 2022
@ivoanjo ivoanjo deleted the ivoanjo/add-note-about-configure-without-a-block branch April 25, 2022 10:19
@github-actions github-actions bot added this to the 1.0.0.beta3 milestone Apr 25, 2022
@marcotc marcotc added the docs Involves documentation label Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Involves documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants