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.

Sign up for GitHub

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

tools: add a backport queue cron action #56143

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

tools: add a backport queue cron action #56143

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6a70cd
tools: add a backport queue cron action
aduh95 Nov 30, 2024
bc2cc72
fixup! tools: add a backport queue cron action
aduh95 Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 169 additions & 0 deletions .github/workflows/backport-queue.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
name: Backport Commit Queue

on:
schedule:
# Run twice a week at 00:15 AM UTC on Sunday and Thursday.
- cron: 15 0 * * 0,4
Copy link
Member

@ruyadorno ruyadorno Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would start with a manually run workflow to give it a try. Also if it keep it a manual step we can easily get around the two weeks problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have to run the workflow manually, then it has no value, I'd rather use the CLI

Copy link
Member

Choose a reason for hiding this comment

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

I see a big value in not having to be at my workstation in order to kickstart a workflow, having the worfklow available in GitHub actions means I can just kickstart it anytime from my phone.

Copy link
Contributor Author

@aduh95 aduh95 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Having it scheduled means you don't even need to pull up your phone though

Copy link
Member

Choose a reason for hiding this comment

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

sure 👍 it makes total sense now that I understand the goal is not to land these commits on staging!

I'd still start with manual runs just to try it out but that's open to you ☺️

workflow_dispatch:

concurrency: ${{ github.workflow }}

env:
NODE_VERSION: lts/*
PYTHON_VERSION: '3.12'
FLAKY_TESTS: keep_retrying
CC: sccache clang
CXX: sccache clang++
SCCACHE_GHA_ENABLED: 'true'

permissions:
contents: read

jobs:
commitQueue:
runs-on: ubuntu-latest
if: github.repository == 'nodejs/node' || github.event_name == 'workflow_dispatch'
strategy:
fail-fast: false
matrix:
branch:
- { release-line: v23.x, base-branch: main }
- { release-line: v22.x, base-branch: v23.x }
Copy link
Member

Choose a reason for hiding this comment

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

we should be carefule with this because v23.x may contain newly released commits that should not be landed during two weeks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's a problem, we'll get notified before the commit is ready whether it lands cleanly on v22.x, it doesn't mean we'll have to backport it right away

Copy link
Member

Choose a reason for hiding this comment

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

Related to my comment above, I think having the workflow be manually ran makes more sense to start with, in that case these values should be parameterized instead.

steps:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
# Needs the whole git history for ncu to work
# See https://github.com/nodejs/node-core-utils/pull/486
fetch-depth: 0
ref: ${{ matrix.branch.release-line }}-staging
# A personal token is required because pushing changes to `.github`
# with default token will be blocked. It needs
# to be set here because `checkout` configures GitHub authentication
# for push as well.
token: ${{ secrets.GH_USER_TOKEN }}

- name: Set up Python ${{ env.PYTHON_VERSION }}
uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0
with:
python-version: ${{ env.PYTHON_VERSION }}

# Install dependencies
- name: Install Node.js
uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0
with:
node-version: ${{ env.NODE_VERSION }}
- name: Install branch-diff
run: npm install -g branch-diff

- name: Setup git author
run: |
git config --local user.email "github-bot@iojs.org"
git config --local user.name "Node.js GitHub Bot"

- name: Set up ghauth config (Ubuntu)
run: |
mkdir -p "${XDG_CONFIG_HOME:-~/.config}/changelog-maker"
echo '{}' | jq '{user: env.USERNAME, token: env.TOKEN}' > "${XDG_CONFIG_HOME:-~/.config}/changelog-maker/config.json"
env:
USERNAME: ${{ secrets.JENKINS_USER }}
TOKEN: ${{ github.token }}

- name: Fetch base branch
run: |
git remote set-branches --add origin "$BASE_BRANCH"
git fetch origin "$BASE_BRANCH"
env:
BASE_BRANCH: ${{ matrix.branch.base-branch }}

- name: Fetch auto-backport branch if it exists
id: fetch
run: |
if git fetch origin "$BACKPORT_BRANCH"; then
STAGING_BRANCH_TIP="$(git rev-parse HEAD)"
WORKING_BRANCH_TIP="$(git rev-parse FETCH_HEAD)"
echo "WORKING_BRANCH_TIP=$WORKING_BRANCH_TIP" >> "$GITHUB_OUTPUT"
if [ "$WORKING_BRANCH_TIP" != "$STAGING_BRANCH_TIP" ]; then
git reset "$WORKING_BRANCH_TIP" --hard
git rebase "$STAGING_BRANCH_TIP" --empty=drop || git reset "$STAGING_BRANCH_TIP" --hard
fi
else
echo "Branch doesn't exist yet"
fi
env:
BACKPORT_BRANCH: ${{ matrix.branch.release-line }}-staging-auto-backport
Copy link
Member

Choose a reason for hiding this comment

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

We have to keep in mind that the purpose of the v<MAJOR>.x-staging branches is precisely to serve as a working area for the releaser, in the sense that the releaser is free to break builds, force-push, and modify it however they want when preparing a release.

With that in mind, adding a second <MAJOR>-staging-auto-backport seems like an unnecessary duplication of effort that I'm not sure is adding value here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I addressed that point in the PR description, we cannot use the commits backported by the automation, so it makes more sense to use a different branch IMO. It could also be on a separate repo if we don't want to clutter nodejs/node with that

Copy link
Member

@ruyadorno ruyadorno Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ok, let me copy/paste each point so that we can discuss each one in more detail:

It's only able to work with commits that cleanly apply to the staging branch, which is trivial for a human to do.

It's not trivial and very time consuming, particularly because these "trivial to cherry-pick" commits are mixed up with many conflicting commits that may or not be simple for the releaser to handle it themselves. This automation should be precisely about separating this two types of commits, whatever lands cleanly can go in the staging branch and whatever needs backport can be handled later - later, as in the next 10mins of work for that releaser or throughout the week, it gives more control to the releaser on how they want to handle it.

No matter how small of a conflict, as soon as a commit doesn't backport it is skipped, and having out of order backports makes branches diverge more and more, making future backport harder.

Agreed, that's why the report generated by nodejs/node-core-utils#875 is in the correct order so that the releaser can do a best-effort work to try and land the easy-to-manage conflicted commits in the right order. Any non-trivial conflict will still require the backport from (likely) the original PR author as usual.

Releasers sign their backport commits. Because commits that land on staging branch receive much fewer attention than the one on main, we don't want anyone to be tempted to introduce subtle backdoor when backporting, having the signature puts the releaser reputation at stake. I don't think we could have the same system with a bot while making sure no one can impersonate the bot.

I think you're talking about the manual backport process when you mention "we don't want anyone to be tempted to introduce subtle backdoor when backporting" - that, has its own process, which consists in opening a new PR that still needs to be reviewed manually (and approved at least by the releaser) and which we usually run tests again, in order to land in the staging branch.

I'm failing to see how the bot impersonation plays out in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm failing to see how the bot impersonation plays out in that case.

If the bot is pushing commits on the staging branch, it would be very easy for a malicious actor to impersonate the bot and push a commit that contains e.g. a backdoor. It's not a problem on main which has a lot of eyes on it, but on staging branches, it would be very hard to spot, and possibly tricky to track down. Currently, the way we have things where only humans are landing stuff on the staging branches puts a good incentive not to be malicious actor, and to be careful not to leak our private keys, as the blame would go to whoever signed the malicious commit.

that, has its own process

I think it would be silly to assume a malicious actor would follow process 🙃


- name: Run the queue
id: queue
run: |
set -xe

NODE="$(command -v node)"

backport() {
git cherry-pick "$1" && \
make lint-js NODE="$NODE" && \
make test-doc -j4 NODE="$NODE"
Comment on lines +104 to +105
Copy link
Member

Choose a reason for hiding this comment

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

The more I think about this problem space, the more I'm convinced that the right way to go about it is to complete the backport queue ignoring tests and automate a git bisect workflow that detects when things broke only if we actually have a broken build in the staging branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running doc tests is quite cheap, I don't see a reason not to

Copy link
Member

Choose a reason for hiding this comment

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

sure thing, that could easily be incorporated into nodejs/node-core-utils#875 too 👍

I'm just making the case that running the full test suite is the most valuable thing and that is very expensive and because of that I've been thinking more and more that bisecting later might actually be the way to go, even for automation.

}

mkdir -p out/Release
ln -s "$NODE" out/Release/node

branch-diff "${CURRENT_RELEASE_LINE}-staging" "$BASE_REF" \
--exclude-label="semver-major,dont-land-on-${CURRENT_RELEASE_LINE},backport-requested-${CURRENT_RELEASE_LINE},backport-blocked-${CURRENT_RELEASE_LINE},backport-open-${CURRENT_RELEASE_LINE},backported-to-${CURRENT_RELEASE_LINE}"\
--filter-release --format=sha --reverse | while read -r COMMIT ; do
if backport "$COMMIT" 2>&1 >output; then
MAX_COMMITS="$((MAX_COMMITS-1))"
else
{
EOF="$(node -p 'crypto.randomUUID()')"
echo "$COMMIT<<$EOF"
echo "…"
tail output # Cutting the output to avoid exceeding memory.
echo "$EOF"
} >> "$GITHUB_OUTPUT"
git cherry-pick --skip || git reset HEAD^ --hard
fi
cat output
[ "$MAX_COMMITS" = "0" ] && break || true
done
rm -f output
env:
MAX_COMMITS: 99 # backport commits 99 at a time to limit the risk of timeout

Check warning on line 131 in .github/workflows/backport-queue.yml

View workflow job for this annotation

GitHub Actions / lint-yaml 

131:27 [comments] too few spaces before comment
CURRENT_RELEASE_LINE: ${{ matrix.branch.release-line }}
BASE_REF: origin/${{ matrix.branch.base-branch }}
BACKPORT_BRANCH: ${{ matrix.branch.release-line }}-staging-auto-backport

- name: Get local HEAD
id: head
run: echo "HEAD=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"

- name: Push to working branch
if: ${{ steps.fetch.outputs.WORKING_BRANCH_TIP != steps.head.outputs.HEAD }}
run: git push origin "HEAD:refs/heads/$BACKPORT_BRANCH" --force
env:
BACKPORT_BRANCH: ${{ matrix.branch.release-line }}-staging-auto-backport

- name: Report errors
if: ${{
steps.fetch.outputs.WORKING_BRANCH_TIP != steps.head.outputs.HEAD &&
toJson(steps.queue.outputs) != '{}'
}}
run: |
node -<<'EOF' | gh issue create --repo "$GITHUB_REPOSITORY_OWNER/Release"\
-t "Autobackport is failing on $CURRENT_RELEASE_LINE" \
--label 'Release-agenda' --body-file -
console.log(`Workflow URL: <${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}>`);
console.log('\n<ul>');
for (const [sha, output] of Object.entries(JSON.parse(process.env.FAILED_BACKPORTS))) {
console.log('<li><details>');
console.log(`<summary><code>${sha}</code></summary>`);
console.log(`\n${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/commit/${sha}\n\n${'```'}\n${output}\n${'```'}\n`);
console.log('</details></li>');
}
console.log('</ul>');
console.log('Try to cherry-pick the commit manually or add `backport-requested` label and leave a comment on the associated PR.')
EOF
env:
GH_TOKEN: ${{ secrets.GH_USER_TOKEN }}
FAILED_BACKPORTS: ${{ toJson(steps.queue.outputs) }}
CURRENT_RELEASE_LINE: ${{ matrix.branch.release-line }}
ruyadorno marked this conversation as resolved.
Show resolved Hide resolved
Loading