Running the tracer_release scenario group on schedule and push to master#290
Running the tracer_release scenario group on schedule and push to master#290
Conversation
nccatoni
force-pushed
the
nccatoni/run-system-tests-tracer_release
branch
from
6661a3b to
16581cd
Compare
nccatoni
force-pushed
the
nccatoni/run-system-tests-tracer_release
branch
from
16581cd to
411965f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #290 +/- ##
=======================================
Coverage 68.88% 68.88%
=======================================
Files 56 56
Lines 7468 7468
Branches 1057 1057
=======================================
Hits 5144 5144
Misses 1818 1818
Partials 506 506 🚀 New features to boost your workflow:
|
dubloom
left a comment
dubloom
left a comment
There was a problem hiding this comment.
LGTM.
Only two questions but this is more me being dumb than me questioning the changes
| secrets: inherit | ||
| secrets: | ||
| DD_API_KEY: ${{ secrets.DD_API_KEY }} | ||
| TEST_OPTIMIZATION_API_KEY: ${{ secrets.DD_API_KEY }} |
There was a problem hiding this comment.
Why do we make that changes ? (like what is the rationale, not we should not do that) ?
There was a problem hiding this comment.
We need the two variables to be populated but they use the same api key here, so instead of duplicating it in the repo config we do this.
There was a problem hiding this comment.
Ohhh makes sense, thank you
| binaries_artifact: binaries | ||
| desired_execution_time: 600 # 10 minutes | ||
| scenarios_groups: appsec | ||
| scenarios_groups: ${{ (github.event_name == 'schedule' || github.event_name == 'push') && 'tracer_release' || 'appsec' }} |
There was a problem hiding this comment.
I'm not sure to understand this.
We want to run the system-tests only when pushing to master and this is a tracer_release or an appsec stuff ?
There was a problem hiding this comment.
It was set up to run only the appsec scenario group on pull requests but we need to run the full tracer_release scenario group on main. I didn't want to change the behavior on pull request so I did this. If you think it is best to run tracer_release for PRs too I'm happy to change it.
PS: The tracer_release scenario group includes appsec and mostly all system-tests making it longer to run than just appsec which was probably the rational behind only activating appsec for pull requests
There was a problem hiding this comment.
With nginx, how much tracer_release would add ? Do we have a lot of test ?
I'd be interested to know if it is taking less or more time than nginx tests because if not, it is better to know at PR stage that we are breaking systemtests. Do you know what the other tracers are doing ?
There was a problem hiding this comment.
From what I could find there are two scenarios that are enable for nginx and not in appsec:
AGENT_SUPPORTING_SPAN_EVENTS
TRACING_CONFIG_NONDEFAULT
It's harder to estimate the runtime increase considering that there is also a small time cost for every deactivated scenario on top of the runtime of the two scenarios mentioned above.
The other tracers don't have a consistent behavior, some use tracer_release some use a custom list of scenario
There was a problem hiding this comment.
Let me ping @xlamorlette-datadog
I have an opinion on it (i'd be good to merge as it is and see if we ever have the issue of breaking system-tests and only see it in main) but I'm not a maintainer so i don't think I should take the decision !
3 participants
Summary
tracer_releasescenario group onscheduleandpushto master (instead ofappsec), while keepingappsecfor pull requestspush_to_test_optimization: trueDD_API_KEYandTEST_OPTIMIZATION_API_KEYexplicitly (replacingsecrets: inherit)Details
The
scenarios_groupsinput is set dynamically based on the event type:schedule/pushto master →tracer_release(broader release-readiness coverage)pull_request→appsec(existing PR checks unchanged)