-
Notifications
You must be signed in to change notification settings - Fork 375
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
Remove CI gem dependency #3288
Remove CI gem dependency #3288
Conversation
68aa670
to
20f3360
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.0 #3288 +/- ##
=======================================
Coverage 98.07% 98.07%
=======================================
Files 1240 1240
Lines 71978 71997 +19
Branches 3391 3392 +1
=======================================
+ Hits 70589 70610 +21
+ Misses 1389 1387 -2 ☔ View full report in Codecov by Sentry. |
Just a bit of a nitpick: This PR is against the branch |
Thank you! only a couple of points:
require 'rspec'
require 'datadog/ci'
# Only activates test instrumentation on CI
if ENV["DD_ENV"] == "ci"
Datadog.configure do |c|
# Configures the tracer to ensure results delivery
c.ci.enabled = true
# The name of the service or library under test
c.service = "my-ruby-app"
# Enables the RSpec instrumentation
c.ci.instrument :rspec
end
end |
20f3360
to
d8e3ef3
Compare
1973958
to
4525edb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for docs!
@@ -145,6 +144,7 @@ def additional_payload_variables | |||
format_configuration_value(configuration.tracing.writer_options[:flush_interval]) | |||
options['logger.instance'] = configuration.logger.instance.class.to_s | |||
options['appsec.enabled'] = configuration.dig('appsec', 'enabled') if configuration.respond_to?('appsec') | |||
options['ci.enabled'] = configuration.dig('ci', 'enabled') if configuration.respond_to?('ci') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still relevant @TonyCTHsu @anmarchenko? I think this might have to be moved to the datadog-ci gem instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what is the meaning of this, maybe it is important for tracer to report the value of ci.enabled to telemetry. Though it will always be false because telemetry is not enabled when CI mode is on, so you might as well remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
I only left one comment, but overall it's in great shape! #3288 (comment)
I have only one comment: this section https://github.com/DataDog/dd-trace-rb/blob/master/docs/GettingStarted.md#ci-visibility still exists in dd-trace-rb docs. I would like us to put there text "For CI visibility use the datadog-ci gem". |
c0b62c9
to
f401d5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good now, thank you!
2.0 Upgrade Guide notes
As of 2.0,
ddtrace
, our CI visibility component has been extracted as a separate gem named datadog-ci, and will no longer be installed together withddtrace
.If you are using our CI visibility product, you will need to include datadog-ci in the project's Gemfile (learn more about the setup)
If you do not want to install datadog-ci, please ensure any CI related configuration under
config.ci.*
is removed.What does this PR do?
This PR removes the datadog-ci dependency from
ddtrace
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!