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

No status report from jobs with matrix if they are skipped #3563

Closed
tnqn opened this issue Mar 31, 2022 · 4 comments · Fixed by #3576
Closed

No status report from jobs with matrix if they are skipped #3563

tnqn opened this issue Mar 31, 2022 · 4 comments · Fixed by #3576
Labels
area/test/infra Issues or PRs related to test infrastructure (Jenkins configuration, Ansible playbook, Kind wrappers kind/bug Categorizes issue or PR as related to a bug.

Comments

@tnqn
Copy link
Member

tnqn commented Mar 31, 2022

Describe the bug

For example, in #3544, there is a status report for "Go / Golangci-lint (pull_request)" (skipped) but no report for "Golangci-lint (macos-latest)" and "Golangci-lint (ubuntu-latest)".

The reason is "Go / Golangci-lint (pull_request)" has matrix defined, but it was skipped before the matrix is expanded but we require the status report of expanded jobs.

There are some discussions on this issue but no solution yet:
actions/runner#952
community/community#9141

The issue causes PRs that have no code change from being merged via the normal process. I'm not sure why there wasn't an issue before. Any idea? @antoninbas

Two workarounds in my mind:

  1. Do not use matrix for required checks.
  2. Move "if" condition to all steps of the matrix jobs so it will first expand then be skipped.

To Reproduce

Create a PR with no code change.

@tnqn tnqn added kind/bug Categorizes issue or PR as related to a bug. area/test/infra Issues or PRs related to test infrastructure (Jenkins configuration, Ansible playbook, Kind wrappers labels Mar 31, 2022
@tnqn
Copy link
Member Author

tnqn commented Mar 31, 2022

I'm not sure why there wasn't an issue before. Any idea? @antoninbas

Is it because antrea-io/has-changes#7 caused has-changes always returned "yes"?

@antoninbas
Copy link
Contributor

I don't think it is because of has-changes. It seems the action is working as expected now.
IMO it is because of #3450, which is also a fairly recent change. Before that change, we never skipped that test for documentation changes, now we do. You could revert that change for the matrix job(s) if you want, and add a comment there about why the job should not be skipped even for doc changes. I am also fine with your other solution (move "if" condition to steps), as this is only one job and it doesn't have too many steps.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 30, 2022
tnqn added a commit to tnqn/antrea that referenced this issue Jul 6, 2022
@tnqn tnqn removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2022
@tnqn
Copy link
Member Author

tnqn commented Jul 6, 2022

@antoninbas I found moving "if" condition to steps caused that the skipped jobs were marked as success which may be misleading. To trade off between unnecessary verification, code redundancy, misleading result, I prefer to sacrifice code redundancy, given anyway the code just repeats once and we don't update them very often.

tnqn added a commit to tnqn/antrea that referenced this issue Jul 6, 2022
Do not use jobs with os matrix when any of the expanded jobs are
required checks, otherwise the reports would be missing if the job is
skipped, causing PRs unmergeable.

Fixes antrea-io#3563

Signed-off-by: Quan Tian <[email protected]>
tnqn added a commit to tnqn/antrea that referenced this issue Jul 6, 2022
Do not use jobs with os matrix when any of the expanded jobs are
required checks, otherwise the reports would be missing if the job is
skipped, causing PRs unmergeable.

Fixes antrea-io#3563

Signed-off-by: Quan Tian <[email protected]>
@tnqn tnqn closed this as completed in #3576 Jul 7, 2022
tnqn added a commit that referenced this issue Jul 7, 2022
Do not use jobs with os matrix when any of the expanded jobs are
required checks, otherwise the reports would be missing if the job is
skipped, causing PRs unmergeable.

Fixes #3563

Signed-off-by: Quan Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test/infra Issues or PRs related to test infrastructure (Jenkins configuration, Ansible playbook, Kind wrappers kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants