Add git detector to telemetry#2409
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 1.x #2409 +/- ##
============================================
+ Coverage 84.94% 85.20% +0.25%
- Complexity 20648 20971 +323
============================================
Files 1570 1588 +18
Lines 63532 64612 +1080
============================================
+ Hits 53968 55050 +1082
+ Misses 9564 9562 -2 🚀 New features to boost your workflow:
|
|
@jdecool I really appreciate the PR but it's a bigger addition that should go through a Proposal Process since there are many architectural choices required in the implementation (like how to call git for example). We can keep this PR and work on it directly, but in the future I would appreciate opening a proper Proposal so we can first talk about how to implement the feature before writing any code |
|
Thanks for your review and your comments. No problem, I'm going to start the proposal process. To explain this PR, I've initially planned to try to add custom detectors dynamically during the PHPUnit process, but I think that git detector could be added by default. But you are right, it's a big change and should be discussed before implementing it. |
I fully agree, git detector should be part of the telemetry, it should be optional though, but also exposed to the whole telemetry not only phpunit bridge
I'm glad we are on the same page! |
|
@jdecool if you would like to brainstorm this first, or have any questions about implementation, scope etc, you can always find me on Flow discord: https://discord.gg/flow-php or you can schedule with me a session online |
|
Thanks @norberttech I've joined the discord. I plan to work on this PR and the proposal process this weekend. |
| @@ -40,6 +41,7 @@ public static function create(Configuration $config): Telemetry | |||
| { | |||
| $telemetryResource = resource_detector() | |||
There was a problem hiding this comment.
I would pass git_detector() here with other default detectors:
resource_detector([
os_detector(),
host_detector(),
process_detector(),
composer_detector(),
environment_detector(),
git_detector()
]);
but ideally (can be handled in next pr) we should probably allow users to configure which detectors they want to use
| * SCP-like SSH remotes (e.g. "git@github.com:org/repo.git") carry no secret and | ||
| * are returned untouched, as are URLs that cannot be parsed. | ||
| */ | ||
| private function sanitizeRepositoryUrl(string $url): string |
There was a problem hiding this comment.
I'm not sure how much I like that sanitization is embedded into GitDetector which seems to be breaking his single responsibility.
I think this whole sanitization logic should be extracted to standalone class, properly unit tested and used in GitDetector as (new RemoteUrlSanitizer())->sanitize($remoteUrl)
|
its looking good @jdecool ! |
2 participants
Change Log
Added
Fixed
Changed
Removed
Deprecated
Security
Description
Sending PHPUnit data is very useful to see what happens during test execution.
But when used in an active project, where there are a lot of tests running, it's very interesting to have Git repository-related information.
So this PR adds a Git detector to attach some VCS information. As it could be interesting outside of the PHPUnit context, I've added it to the telemetry module.