Appsync/fix ci

[cloutierMat]

Motivation

I realized we had security warnings related to the cdk directory. It also lead me to realise that the ci has been broken for a long time. I had assumed the pr were auto merge only when the test were green so never really bothered to have a second look.

Along with #185 this pr fixes this issue

while #185 fixes the tests themselves as no longer compatible with latest versions, this pr aims to prevent this from happening again.

Changes

• Dependabot will now also look to update packages in /cdk
• upgraded node version to match LocalStack node version
• Ensure we don't merge if tests are failing and write a message to our attention instead
• delay the auto-merge workflow run to after the tests are executed using on: workflow_run

[simonrw]

I trust that this is an improvement, but I think there's a simpler way to support automerging. I think the issue is more that we don't have any branch protection rules, so the "auto merge" logic sees the tests pass, that no tests are required to pass, and therefore it merges the PR. If we put a protection rule on the main branch that states that PRs must be green and jobs a, b, and c are required to pass before the PR is green then the auto-merge system should work. WDYT?

[simonrw]

IDK if this works, but...

Suggested change

	TEST_IMAGE_NAME: public.ecr.aws/lambda/nodejs:22
	NODE_VERSION: 22
	NODE_VERSION: 22
	TEST_IMAGE_NAME: public.ecr.aws/lambda/nodejs:${NODE_VERSION}

[simonrw]

This is pretty telling... 😂

[cloutierMat]

The issue with adding branch protection rules, is that the auto-merge runs on:pull-request which mean it would simply fail, as it currently auto-merges as soon as created.

Looking a bit more into it now, we can instead use on: workflow_run, which will run it only after the run as completed. Much better than my current fix to wait that it completes. Thanks for the pushback leading to more thought process 🧠 😄

[simonrw]

This latest change is a security issue.

I don't fully understand your message about using protection rules. My understanding of the flow is:

1. Dependabot creates a PR updating dependencies
2. The dependabot-automerge workflow runs on this PR marking the PR as ready for merging once CI is green, but only if the actor is dependabot
3. The deps update PR actions finish (and are green), then GitHub auto-merges the PR

Do you mean there is a race condition between steps 2 and 3?

[simonrw]

issue: we should not use workflow run. It runs in a privileged context, and I think can be triggered from fork PRs. The user could add a malicious GitHub Actions workflow (or make a change to this one) and it would run with access to our secrets.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#workflow_run

[cloutierMat]

oh, I was not aware of the --auto behavior. I was assuming it would try to merge right away, as it was the current behavior. Now your flow makes sense. 👍

[cloutierMat]

@simonrw, Now that I understand the behavior of gh pr merge --auto, your solution makes total sense. I now reverted the changes to automerge and took the opportunity for a couple upgrade. dependabot/fetch-metadata@v1 => v2. v3 just got released, but requires node 24. Currently, ubuntu-latest is packaged with node 20.

The changes to the file were to reflect the current recommended action

[simonrw]

Looks great, thanks for pushing through my nits!

I reserve the right to be wrong however about anything I said!

[simonrw]

nice additional safety check here!

[cloutierMat]

I reserve the right to be wrong however about anything I said!

I always give you that benefit 😉, but in this case, it turns out I was wrong 😨 🎉

Thanks for the diligence in your review, helping us keeping thing simple and secured 🙏