Skip to content
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

Fix Git default branch #602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix Git default branch #602

wants to merge 2 commits into from

Conversation

giggsey
Copy link
Contributor

@giggsey giggsey commented Jul 27, 2022

Newer versions of Git set the default branch to 'main', which breaks the tests (as SensioLabsSecurityChecker uses master).

The https://github.com/FriendsOfPHP/security-advisories repo still uses master, so we can't change anything within the actual service yet.

Newer versions of Git set the default branch to 'main', which breaks the tests (as SensioLabsSecurityChecker uses master).

The https://github.com/FriendsOfPHP/security-advisories repo still uses master, so we can't change anything within the actual service yet.
@xvilo
Copy link
Contributor

xvilo commented Sep 6, 2022

Wonderful MR, can you do the following thing @giggsey:

There were 2 errors:
1) Buddy\Repman\Tests\Unit\Service\Security\SensioLabsSecurityCheckerTest::testUpdateWhenRepoDontExist
Symfony\Component\Process\Exception\ProcessFailedException: The command "'git' 'clone' '--depth' '1' '--branch' 'master' 'file:///tmp/repman/security-advisories-repo' '.'" failed.
Exit Code: 128(Invalid exit argument)
Working directory: /tmp/repman/security-advisories
Output:
================
Error Output:
================
Cloning into '.'...
fatal: '/tmp/repman/security-advisories-repo' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
/buddy/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:218
/buddy/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:196
/buddy/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:34
/buddy/repman/tests/Unit/Service/Security/SensioLabsSecurityCheckerTest.php:119
2) Buddy\Repman\Tests\Unit\Service\Security\SensioLabsSecurityCheckerTest::testUpdateWhenRepoExist
Symfony\Component\Process\Exception\ProcessFailedException: The command "'git' 'clone' '--depth' '1' '--branch' 'master' 'file:///tmp/repman/security-advisories-repo' '.'" failed.
Exit Code: 128(Invalid exit argument)
Working directory: /tmp/repman/security-advisories
Output:
================
Error Output:
================
Cloning into '.'...
fatal: '/tmp/repman/security-advisories-repo' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
/buddy/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:218
/buddy/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:196
/buddy/repman/src/Service/Security/SecurityChecker/SensioLabsSecurityChecker.php:34
/buddy/repman/tests/Unit/Service/Security/SensioLabsSecurityCheckerTest.php:134
ERRORS!
Tests: 496, Assertions: 1194, Errors: 2.

@giggsey
Copy link
Contributor Author

giggsey commented Sep 6, 2022

From memory, the test failure error is actually misleading. It would probably have been better if I changed the line from ->run() to ->mustRun(), as the git init would have failed on the Buddy CI system.

I believe that is because the version of Git used is out of date, and does not support the initial branch param.

@xvilo
Copy link
Contributor

xvilo commented Sep 6, 2022

Possibly it would be better to download the repository as a zip, then just unpacking it? This way Repman does not have to rely on external programs

@giggsey
Copy link
Contributor Author

giggsey commented Sep 6, 2022

Possibly, but the actual running of repman relies on Git working locally anyway, so I don't see the major harm in running Git here too.

Updating the tests to the latest version of PHP 7.4 might resolve it anyway, it currently uses library/php:7.4.1

@akondas
Copy link
Member

akondas commented Sep 10, 2022

I think we can use new composer audit command. WDYT?

@giggsey
Copy link
Contributor Author

giggsey commented Sep 10, 2022 via email

@akondas
Copy link
Member

akondas commented Sep 10, 2022

Does it cache anything though? Wouldn't want to hit their API for every package within repman. Possibly using their API directly might work better and cache it similar to the git pull now

that's why I'm asking, because from my point of view, we can leave what we have now

@giggsey
Copy link
Contributor Author

giggsey commented Sep 22, 2022

that's why I'm asking, because from my point of view, we can leave what we have now

My view is to leave it pulling from the repo for the time being. There are other areas of repman that could do with the attention that direct improve people's use of it, rather than refactoring a background task

@xvilo
Copy link
Contributor

xvilo commented Jul 9, 2023

We should still need this merged as it indeed has issues on newer PHP docker images and with the default git of macos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants