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

ci: only report status after matrix jobs are done #23865

Merged
merged 1 commit into from Mar 22, 2023

Conversation

spacewander
Copy link
Contributor

@spacewander spacewander commented Feb 18, 2023

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #23729

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 18, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 18, 2023
@spacewander
Copy link
Contributor Author

The solution is found at https://docs.github.com/en/actions/using-jobs/using-jobs-in-a-workflow.

And its effect's similar to:
微信截图_20230218213054
微信截图_20230218213136

Currently I only update one CI workflow. If this one is accepted, I will update the remain workflows in this PR.

@spacewander spacewander marked this pull request as ready for review February 18, 2023 14:24
@spacewander spacewander requested review from a team as code owners February 18, 2023 14:24
@aanm aanm requested a review from nbusseneau February 20, 2023 15:37
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

@spacewander thank you for these changes. Does this work? I'm only asking because as far as I know the if: ${{ success() }}, if: ${{ failure() }} and if: ${{ cancelled() }} only refer to the steps from the same job, not from previous jobs.

@spacewander
Copy link
Contributor Author

@spacewander thank you for these changes. Does this work? I'm only asking because as far as I know the if: ${{ success() }}, if: ${{ failure() }} and if: ${{ cancelled() }} only refer to the steps from the same job, not from previous jobs.

Yes. GitHub documents this behavior at https://docs.github.com/en/actions/using-jobs/using-jobs-in-a-workflow.
I also tested it in my repo, the result can be seen from the screenshots in #23865 (comment)

@nebril nebril added the release-note/misc This PR makes changes that have no direct user impact. label Feb 21, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 21, 2023
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Unfortunately, I don't think this works as intended, as per the documentation: https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions

The status check functions only work for steps in the same job, as André was mentioning. Snippet from the doc (and same for other functions):

success
Returns true when none of the previous steps have failed or been canceled.

The failure one works with ancestor jobs, but I don't think it works for the others.

@nbusseneau
Copy link
Member

nbusseneau commented Feb 21, 2023

I think for checking against previous jobs, we should instead use needs.foo.result == 'success' (with foo being the ancestor job): https://docs.github.com/en/actions/learn-github-actions/contexts#needs-context

@spacewander
Copy link
Contributor Author

Although GitHub's doc may not make it clear, the success can be used for previous job if the if condition is applied at the job level. Here is an example: https://github.com/spacewander/play_with_travis/blob/master/.github/workflows/ci.yml, the run result can be found in https://github.com/spacewander/play_with_travis/actions/runs/4238632888.

微信截图_20230222101520

We can see the "Set commit status to success" job is run after the matrix job "build" is successful.

@aanm aanm requested a review from nbusseneau March 6, 2023 13:08
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Nice then! In this case we need for the reporting job to have access to the SHA, otherwise the commit status API step itself will fail (as is the case right now in the examples provided).

The following step needs to be added prior to calling the commit status API to retrieve the SHA:

      - name: Set up job variables
        id: vars
        run: |
          if [ ${{ github.event.issue.pull_request || github.event.pull_request }} ]; then
            PR_API_JSON=$(curl \
              -H "Accept: application/vnd.github.v3+json" \
              -H "Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}" \
              ${{ github.event.issue.pull_request.url || github.event.pull_request.url }})
            SHA=$(echo "$PR_API_JSON" | jq -r ".head.sha")
          else
            SHA=${{ github.sha }}
          fi

@brb
Copy link
Member

brb commented Mar 15, 2023

@spacewander 👋 Are you still on this PR?

@maintainer-s-little-helper
Copy link

Commit b93da8ff449381b2611f0d1b6b03981332a3f819 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 16, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 16, 2023
@brb
Copy link
Member

brb commented Mar 16, 2023

@spacewander Thanks! Could you squash your commits into a single one?

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@spacewander
Copy link
Contributor Author

@spacewander Thanks! Could you squash your commits into a single one?

Done

@brb brb requested review from nbusseneau and aanm March 17, 2023 14:17
@nbusseneau
Copy link
Member

Thanks a lot, LGTM.

@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 21, 2023
@borkmann borkmann merged commit 683b101 into cilium:master Mar 22, 2023
40 of 41 checks passed
giorio94 added a commit to giorio94/cilium that referenced this pull request Apr 6, 2023
This commit applies the same changes introduced in cilium#23865 to the
ci-multicluster workflow, moving the status reporting after all matrix
jobs are done. This prevents that the status reported by one job is
later overwritten by a subsequent one.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94 added a commit that referenced this pull request Apr 6, 2023
This commit applies the same changes introduced in #23865 to the
ci-multicluster workflow, moving the status reporting after all matrix
jobs are done. This prevents that the status reported by one job is
later overwritten by a subsequent one.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
gandro added a commit that referenced this pull request Apr 11, 2023
Before this commit, the ci-datapath workflow would update the PR status
for each `setup-and-test` matrix job. That however lead to the PR being
marked as green as soon as the first matrix job succeeded, even though
later tests might fail.

Worse, if an earlier matrix job failed, and a later one succeeded, the
latter job masked the failure of the earlier job and marked the PR as
green as well.

This commit fixes the issue by only updating the commit status once all
jobs have run. This technique is taken from #23865 and has
been used for the BPF verifier workflow for a while.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
dylandreimerink pushed a commit that referenced this pull request Apr 12, 2023
Before this commit, the ci-datapath workflow would update the PR status
for each `setup-and-test` matrix job. That however lead to the PR being
marked as green as soon as the first matrix job succeeded, even though
later tests might fail.

Worse, if an earlier matrix job failed, and a later one succeeded, the
latter job masked the failure of the earlier job and marked the PR as
green as well.

This commit fixes the issue by only updating the commit status once all
jobs have run. This technique is taken from #23865 and has
been used for the BPF verifier workflow for a while.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
dylandreimerink pushed a commit that referenced this pull request Apr 13, 2023
This commit applies the same changes introduced in #23865 to the
ci-multicluster workflow, moving the status reporting after all matrix
jobs are done. This prevents that the status reported by one job is
later overwritten by a subsequent one.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Premature GitHub status report in case of matrix jobs
6 participants