-
Notifications
You must be signed in to change notification settings - Fork 411
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
chore(civis): move configurations to envier #10969
base: main
Are you sure you want to change the base?
Conversation
|
2011ae5
to
ccd6ebb
Compare
ccd6ebb
to
a14cae0
Compare
Datadog ReportBranch report: ✅ 0 Failed, 1286 Passed, 0 Skipped, 13m 23.17s Total Time |
TODO: fix how ci_vis configurations are mocked in tests |
BenchmarksBenchmark execution time: 2024-10-09 06:35:11 Comparing candidate commit 2c5c483 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 372 metrics, 53 unstable metrics. scenario:span-start
|
9af783c
to
adbc139
Compare
ddtrace/settings/civis.py
Outdated
class CIVisConfig(En): | ||
__prefix__ = "dd.civisibility" | ||
|
||
_itr_enabled = En.v( |
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.
what's the underscore for?
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 want to keep this attriubte "internal" to keep things consistent with how this confiuration was previously defined. Not sure if this is the right approach
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.
If the corresponding env var is "private" (i.e. has a _
prefix, then it should have the private=True
kwarg (e.g.
dd-trace-py/ddtrace/settings/symbol_db.py
Line 32 in f757fbf
private=True, |
_
in here as well
ddtrace/settings/civis.py
Outdated
_agentless_enabled = En.v( | ||
bool, | ||
"agentless.enabled", | ||
default=False, | ||
help_type="Boolean", | ||
help="Enable ....", | ||
) | ||
|
||
_agentless_url = En.v( | ||
str, | ||
"agentless.url", | ||
default="", | ||
help_type="String", | ||
help="Enable ....", | ||
) |
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.
These could be grouped in a nested sub-class
Motivation
Changes
ci_visibility_log_level
andtest_session_name
configurations fromddtrace.config
toddtrace.settings.civis.ci_config
andddtrace.settings.civis.test_config
. Since support forci_visibility_log_level
andtest_session_name
is unreleased this is not a breaking change.ddtrace.config
(the remaining configs are internal so this is not a breaking change)Checklist
Reviewer Checklist