-
Notifications
You must be signed in to change notification settings - Fork 408
Report agentic onboarding marker in configuration telemetry (RFC-1113) #6079
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
base: master
Are you sure you want to change the base?
Changes from all commits
d3aa693
b9ea772
d849cfb
d1026f2
252ac05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,14 @@ def self.add_settings!(base) | |
| o.default false | ||
| end | ||
|
|
||
| # Set by the agentic onboarding solution; no behavior, reported verbatim in | ||
| # configuration telemetry only (RFC-1113). | ||
| option :agentic_onboarding do |o| | ||
| o.type :string | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests set the value to a boolean, which means the type should probably be bool and not string.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is defined as a string, both in the RFC and the registry
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also note that the CI shouldn't pass if the configuration was not defined as the registry is. |
||
| o.env "DD_APPSEC_AGENTIC_ONBOARDING" | ||
| o.default "" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was the reasoning for using an empty string instead of nil? Or false if the type is indeed supposed to be bool?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. null is not always reported, empty string is. |
||
| end | ||
|
|
||
| define_method(:instrument) do |integration_name| | ||
| if enabled | ||
| registered_integration = Datadog::AppSec::Contrib::Integration.registry[integration_name] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -434,6 +434,38 @@ def patcher | |
| end | ||
| end | ||
|
|
||
| describe "#agentic_onboarding" do | ||
| subject(:agentic_onboarding) { settings.appsec.agentic_onboarding } | ||
|
|
||
| context "when DD_APPSEC_AGENTIC_ONBOARDING" do | ||
| around do |example| | ||
| ClimateControl.modify("DD_APPSEC_AGENTIC_ONBOARDING" => agentic_onboarding_env) do | ||
| example.run | ||
| end | ||
| end | ||
|
|
||
| context "is not defined" do | ||
| let(:agentic_onboarding_env) { nil } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I commented on the RFC. This test case converts the "not present" case (env var is nil) to "present" case (env var is set to empty string). The RFC specifies that the value of the env var is ignored, therefore the test missing from the PR is env var is set to empty string -> reported value is true. |
||
|
|
||
| it { is_expected.to eq("") } | ||
| end | ||
|
|
||
| context "is defined" do | ||
| let(:agentic_onboarding_env) { "true" } | ||
|
|
||
| it { is_expected.to eq("true") } | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe "#agentic_onboarding=" do | ||
| subject(:set_agentic_onboarding) { settings.appsec.agentic_onboarding = "true" } | ||
|
|
||
| before { set_agentic_onboarding } | ||
|
|
||
| it { expect(settings.appsec.agentic_onboarding).to eq("true") } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The RFC specifies that telemetry signal is a boolean. Therefore the string -> bool conversion should probably be done at configuration parsing time and this test should be written to expect a boolean value. |
||
| end | ||
|
|
||
| describe "#obfuscator_key_regex" do | ||
| subject(:obfuscator_key_regex) { settings.appsec.obfuscator_key_regex } | ||
|
|
||
|
|
||
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.
This looks like AI slop. The comment (or the rest of PR) does not provide a description of what the value actually contains.
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.
Some people wants no comments, you seem to want all the explanation in here ? The Appsec RFC ID is put here in reference, so anyone can take a look at it for more explanations.
Uh oh!
There was an error while loading. Please reload this page.
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.
Please remove the RFC reference and JIRA numbers from code comment and pull request description and title.
Replace with an example how agentic onboarding solution is going to modify this specific settings. Provided with possible values.