Add git detector to telemetry#2409
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 1.x #2409 +/- ##
=========================================
Coverage 84.87% 84.87%
- Complexity 20582 20594 +12
=========================================
Files 1567 1568 +1
Lines 63194 63226 +32
=========================================
+ Hits 53633 53664 +31
- Misses 9561 9562 +1 🚀 New features to boost your workflow:
|
| @shell_exec('mkdir -p ' . escapeshellarg($directory)); | ||
| @shell_exec($git . ' init -q 2>/dev/null'); | ||
| @shell_exec($git . ' config user.email "test@example.com" 2>/dev/null'); | ||
| @shell_exec($git . ' config user.name "Test" 2>/dev/null'); | ||
| @shell_exec($git . ' commit --allow-empty -m "init" -q 2>/dev/null'); | ||
|
|
||
| if ($branch !== null) { | ||
| @shell_exec($git . ' branch -c ' . escapeshellarg($branch) . ' 2>/dev/null'); | ||
| @shell_exec($git . ' checkout -q ' . escapeshellarg($branch) . ' 2>/dev/null'); | ||
| } | ||
|
|
||
| if ($tag !== null) { | ||
| @shell_exec($git . ' tag ' . escapeshellarg($tag) . ' 2>/dev/null'); | ||
| @shell_exec($git . ' checkout -q ' . escapeshellarg($tag) . ' 2>/dev/null'); | ||
| } |
There was a problem hiding this comment.
I'm honestly not very comfortable with all those shell execs in the test suite.
I'm also tryign to use local __DIR__ . /var folders close to tests rather than sys_get_temp_dir.
Personally I think it would be better if those git repositories would live in some Fixtures folder in the integration test suite instead of creating them on demand
| private function runGit(string $arguments): ?string | ||
| { | ||
| $command = 'git '; | ||
|
|
||
| if ($this->workingDirectory !== null) { | ||
| $command .= '-C ' . escapeshellarg($this->workingDirectory) . ' '; | ||
| } | ||
|
|
||
| $command .= $arguments . ' 2>/dev/null'; | ||
|
|
||
| $output = @shell_exec($command); | ||
|
|
||
| if (!is_string($output)) { | ||
| return null; | ||
| } | ||
|
|
||
| $output = trim($output); | ||
|
|
||
| return $output === '' ? null : $output; |
There was a problem hiding this comment.
let me think about this one, it might expose some security details.
For example here: $repositoryUrl = $this->runGit('git remote get-url origin');
If one is keeping it like this https://user:password@host/repo.git.
It also seems that $this->runGit('git remote get-url origin'); would create git git remote get-url origin command.
Another issue I see here is that git is resolved from $PATH which might not be the always the case so users should be able to pass also additional git binary path.
I'm also not sure that shell_exec is the best option here, I would probably try to do it with proc_open as it does not need to even go through shell
private function runGit(array $args): ?string
{
$cmd = ['git'];
if ($this->workingDirectory !== null) {
$cmd[] = '-C';
$cmd[] = $this->workingDirectory;
}
$cmd = [...$cmd, ...$args];
// run with proc_open / Process, no shell
}
|
@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 |
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.