-
Notifications
You must be signed in to change notification settings - Fork 114
Update LinuxApplicationLayerExperiment control group #1616
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
Update LinuxApplicationLayerExperiment control group #1616
Conversation
1c68024 to
37fa219
Compare
|
👋 Hi! It looks like you modified some files in the
If none of the above scenarios apply, feel free to ignore this comment 🙂 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1616 +/- ##
=====================================
Coverage 90.1% 90.1%
=====================================
Files 435 435
Lines 37358 37376 +18
Branches 2309 2310 +1
=====================================
+ Hits 33667 33684 +17
Misses 3217 3217
- Partials 474 475 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR removes npm, pip, and NuGet component detectors from the control group of the LinuxApplicationLayerExperiment. These detectors were incorrectly included in the experiment's control group, causing the experiment to incorrectly report that these components were being removed when they are not actually planned for removal.
Changes:
- Simplified the
IsInControlGroupmethod to only includeLinuxContainerDetector(excludingLinuxApplicationLayerDetector) - Removed unnecessary using statements for npm, pip, and NuGet detector namespaces
- Removed test cases that verified npm, pip, and NuGet detectors were in the control/experiment groups
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/LinuxApplicationLayerExperiment.cs |
Simplified control group logic to only include Linux container detector; removed unused imports |
test/Microsoft.ComponentDetection.Orchestrator.Tests/Experiments/LinuxApplicationLayerExperimentTests.cs |
Removed test cases for npm, pip, and NuGet detectors; removed unused imports |
Comments suppressed due to low confidence (1)
src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs/LinuxApplicationLayerExperiment.cs:9
- The XML documentation comment is now outdated. It states that the control group includes 'standard file-based npm and pip detectors' but these have been removed from the control group. The comment should be updated to reflect that only the Linux system package detector is in the control group.
/// Control group includes the standard file-based npm and pip detectors plus the Linux system package detector.
|
I don't think we should update the experiment. This detector is introducing packages from multiple ecosystems, we need to know exactly what it found in all of them, that the other detectors couldn't report or vice versa. |
37fa219 to
b9bbadf
Compare
b9bbadf to
111b622
Compare
111b622 to
612af46
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Updates the
LinuxApplicationLayerExperimentto correctly measure net-new application packages found exclusively inside containers (e.g., packages installed via RUN npm add lodash).Changes
Experiment Configuration:
This setup ensures:
ShouldRecord Logic: