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

Auto-detect Github Actions CI and activate github logger accordingly #1645

Merged
merged 15 commits into from May 15, 2022

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Jan 11, 2022

This PR:

Fixes #1560


With this PR, the --logger-github flag is not necessary when running in Github Actions anymore. It's still supported:

  • doesn't break existing scripts
  • can be used to force-enable this particular logger in non-Github Action environments if desired

When running on Github Actions, however, it's not possible to "disable" this logger; as long as the Github Actions CI is correctly detected according to OndraM/ci-detector, it will always be active. This is deliberately done to improve the out of the box experience with infection.

@mfn mfn marked this pull request as ready for review January 11, 2022 21:23
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@maks-rafalko maks-rafalko added this to the next milestone Jan 12, 2022
@maks-rafalko
Copy link
Member

Build failure

The minimum required MSI percentage should be 71.87%, but actual is 71.86%. Improve your tests!

it's not your fault, but comes from master. You can however change this line https://github.com/infection/infection/blob/master/.github/workflows/mt.yaml#L14 and update it to 71.86 so builds will pass.

Anyway, I'm happy to merge, please update the docs.

mfn added a commit to mfn/infection that referenced this pull request Jan 12, 2022
@mfn
Copy link
Contributor Author

mfn commented Jan 12, 2022

Thanks! I've pushed three more changes:

@maks-rafalko
Copy link
Member

maks-rafalko commented Jan 12, 2022

looks like our end-to-end tests are broken now (https://github.com/infection/infection/runs/4787795564?check_suite_focus=true#step:12:5)

I suspect this is because tests, executed from PHPUnit, started producing GitHub warning messages (https://github.com/infection/infection/runs/4787795564?check_suite_focus=true#step:12:27) and this wasn't exptected from PHPUnit context

I have a feeling that we need to disable github logger here. Since it's being enabled automatically, probably we need to make it possible to pass --logger-github=false here:

'--no-interaction',

so the logic would be:

  • GHA context, no options - automatically enabled
  • GHA context, --logger-github=false - disabled
  • GHA context, --logger-github - enabled
  • non-GHA context, no options - disabled
  • non-GHA context, --logger-github=false - disabled
  • non-GHA context, --logger-github - enabled

what do you think?

Let me know if you need any help, since e2e tests can be not very clear.

@mfn
Copy link
Contributor Author

mfn commented Jan 30, 2022

@maks-rafalko what a gotcha 😅

I've pushed a commit to support --logger-github=false, from the commit

TL;DR:

  • not provided: auto-detected
  • --logger-github : enabled
  • --logger-github=true : enabled
  • --logger-github=false : disabled

I hope this is what you had in mind; I had to change it to nullable etc.

Though I've to trouble to figure out where to explicitly put --logger-github=false; somewhere in .github/workflows/ci.yaml I presume?

Comment on lines 337 to 350
private function detectCiGithubActions(): bool
{
try {
$ci = $this->ciDetector->detect();

if ($ci->getCiName() === CiDetector::CI_GITHUB_ACTIONS) {
return true;
}
} catch (CiNotDetectedException $e) {
// deliberately empty
}

return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private function detectCiGithubActions(): bool
{
try {
$ci = $this->ciDetector->detect();
if ($ci->getCiName() === CiDetector::CI_GITHUB_ACTIONS) {
return true;
}
} catch (CiNotDetectedException $e) {
// deliberately empty
}
return false;
}
private function detectCiGithubActions(): bool
{
try {
$ci = $this->ciDetector->detect();
} catch (CiNotDetectedException $e) {
return false;
}
if ($ci->getCiName() !== CiDetector::CI_GITHUB_ACTIONS) {
return false;
}
return true;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

Comment on lines 454 to 457
throw new InvalidArgumentException(
sprintf('Cannot pass "%s" to "--%s": only "true", "false" or no argument is supported', $useGitHubLogger, self::OPTION_LOGGER_GITHUB)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new InvalidArgumentException(
sprintf('Cannot pass "%s" to "--%s": only "true", "false" or no argument is supported', $useGitHubLogger, self::OPTION_LOGGER_GITHUB)
);
throw new InvalidArgumentException(sprintf(
'Cannot pass "%s" to "--%s": only "true", "false" or no argument is supported',
$useGitHubLogger,
self::OPTION_LOGGER_GITHUB
));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

@maks-rafalko
Copy link
Member

@mfn could you please rebase?

mfn added a commit to mfn/infection that referenced this pull request Jan 31, 2022
@mfn
Copy link
Contributor Author

mfn commented Jan 31, 2022

@mfn could you please rebase?

Done and force-pushed!

@maks-rafalko
Copy link
Member

I've rebased and fixed e2e tests.

Thanks for this feature @mfn!

@maks-rafalko maks-rafalko merged commit 38004b5 into infection:master May 15, 2022
@mfn mfn deleted the mfn-logger-detect-ci branch May 15, 2022 17:26
@mfn
Copy link
Contributor Author

mfn commented May 15, 2022

Thank you for your time too 🙏🏼

@maks-rafalko
Copy link
Member

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

Successfully merging this pull request may close these issues.

Automatically activate --logger-github in github CI
3 participants