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: Add a call to the update label backport action #29902

Merged
merged 1 commit into from Jan 16, 2024
Merged

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Dec 14, 2023

Add an action to call the workflow that update the labels of backported
PRs in all stable branches.

This commit is based on the following commits by Fabio from v1.14 branch:

  • 81ade5f ("ci: Call the workflow to update labels of backported PRs")
  • a5a047f ("ci: Use pull_request_target in update label workflow")

The primary change here is to list all maintained branches in a single
workflow on main in order to simplify the maintenance burden when
creating new stable branches (eg, during v1.15 stable branch creation).

This action will not trigger from the main branch for PRs targeted to
stable branches. However, when we copy this workflow to stable branches,
it will run for PRs targeted to that stable branch (assuming that the
versions referenced in this file are kept in sync with the branch
version).

Related: #27875

Tested here: https://github.com/pippolo84/gh-action-label-test/actions/runs/6773033350/job/18406926291

@joestringer joestringer requested review from a team as code owners December 14, 2023 22:51
@joestringer joestringer added release-note/ci This PR makes changes to the CI. dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. labels Dec 14, 2023
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

I've experimented with this a bit in a test repository.

Here the final version of the relevant workflow.

I made some changes to avoid the usage of ${{ matrix.branch }} in the if condition. Reading the docs it seems that the condition is evaluated before the matrix expansion, so it is not possible to write it like that. Relevant excerpt from the docs:

Note: The jobs.<job_id>.if condition is evaluated before jobs.<job_id>.strategy.matrix is applied.

That workflow seems to work correctly, but unfortunately, it doesn't when its file is only on the main branch. I think that's related to what is explained here:

GitHub searches the .github/workflows directory in your repository for workflow files that are present in the associated commit SHA or Git ref of the event.

I think that in our case (a PR with a stable branch as target) the workflow file is searched in the target branch, and since it is not found, it isn't triggered.

@joestringer joestringer marked this pull request as draft January 5, 2024 02:51
@joestringer
Copy link
Member Author

joestringer commented Jan 5, 2024

Oh, awesome thanks @pippolo84 . If I follow your response right, I've made a mistake here in this PR, thinking that GitHub workflows on main can trigger on PRs merged into stable branches, but that's not the case. When I look through the other workflows on main, I don't see any examples that try to do what I'm doing in the initial iteration of this PR. There's some ariane or other configuration but I think they're invoked from a workflow on another branch, so it doesn't quite fit the pattern I was attempting.

That workflow seems to work correctly, but unfortunately, it doesn't when its file is only on the main branch

We don't need to call the label updater on PRs to the main branch though right? Because backports only go to stable branches?

Here the final version of the relevant workflow.

Let's say we merge this version of the workflow. It won't match for PRs to stable branches, because this only lives on main. However, if we backport this exact workflow to v1.15 or earlier, it will now trigger for backport PRs targeted to that stable branch. Similarly, when we prepare the next stable branch, we would not need to customize this action at all. As long as the branch list on line 15 is kept up-to-date on main with the three latest stable branches, this would simplify the maintenance of the workflow going forward.

Is that your understanding as well, or do you think I missed something?

@pippolo84
Copy link
Member

We don't need to call the label updater on PRs to the main branch though right? Because backports only go to stable branches?

Yep, that's correct. 👍

Let's say we merge this version of the workflow. It won't match for PRs to stable branches, because this only lives on main. However, if we backport this exact workflow to v1.15 or earlier, it will now trigger for backport PRs targeted to that stable branch.

👍

Similarly, when we prepare the next stable branch, we would not need to customize this action at all. As long as the branch list on line 15 is kept up-to-date on main with the three latest stable branches, this would simplify the maintenance of the workflow going forward.

Is that your understanding as well, or do you think I missed something?

That's my understanding too. 👍
The new version of the workflow is easier to maintain because only that matrix need to be updated before cutting the next stable branch from main. Then, the workflow file in the stable branch will do the job, while the one in the main branch will be just a placeholder.

Just a note: if you intend to go ahead with this version, beware of the call at line 33, it should be changed to be:

uses: cilium/cilium/.github/workflows/update-label-backport-pr.yaml@main

@joestringer joestringer added needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch and removed dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. labels Jan 5, 2024
@joestringer joestringer marked this pull request as ready for review January 5, 2024 18:46
@joestringer
Copy link
Member Author

@pippolo84 I've synced to your version, updated the versions + target branch for the action, and updated the commit metadata to reflect your contribution to this PR. I think this should be ready for review now.

Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

🚀

@joestringer
Copy link
Member Author

I saw an optional job fail here:
https://github.com/cilium/cilium/actions/runs/7425540767/job/20207405170?pr=29902

Applying this incremental change to the patch to fix it:

diff --git a/.github/workflows/call-backport-label-updater.yaml b/.github/workflows/call-backport-label-updater.yaml
index 99474eae646d..fb4fe356ab38 100644
--- a/.github/workflows/call-backport-label-updater.yaml
+++ b/.github/workflows/call-backport-label-updater.yaml
@@ -9,6 +9,7 @@

   jobs:
     get-branch:
+      name: Detect base branch
       runs-on: ubuntu-latest
       strategy:
         matrix:
@@ -28,6 +29,7 @@
             fi

     call-backport-label-updater:
+      name: Update backport labels for upstream PR
       needs: get-branch
       if: ${{needs.get-branch.outputs.version}} != ''
       uses: cilium/cilium/.github/workflows/update-label-backport-pr.yaml@main

Add an action to call the workflow that update the labels of backported
PRs in stable branch.

This commit is based on the following commits by Fabio from v1.14 branch:
- 81ade5f ("ci: Call the workflow to update labels of backported PRs")
- a5a047f ("ci: Use pull_request_target in update label workflow")

The primary change here is to list all maintained branches in a single
workflow on main in order to simplify the maintenance burden when
creating new stable branches (eg, during v1.15 stable branch creation).

This action will not trigger from the main branch for PRs targeted to
stable branches. However, when we copy this workflow to stable branches,
it will run for PRs targeted to that stable branch (assuming that the
versions referenced in this file are kept in sync with the branch
version).

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Co-authored-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer
Copy link
Member Author

/test

@aanm aanm merged commit 7fc78e9 into main Jan 16, 2024
196 of 204 checks passed
@aanm aanm deleted the pr/joe/backport-labeler branch January 16, 2024 09:18
@gandro gandro mentioned this pull request Jan 16, 2024
6 tasks
@gandro gandro added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 16, 2024
@gandro gandro added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jan 16, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.15 in v1.15.0-rc.1 Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. release-note/ci This PR makes changes to the CI.
Projects
No open projects
v1.15.0-rc.1
Backport done to v1.15
Development

Successfully merging this pull request may close these issues.

None yet

7 participants