Update ComposerLockDiff workflow to run without DDEV and use a sticky pull request comment #538
Conversation
…l request comment to post results.
davereid
force-pushed
the
composer-lock-diff-sticky-pr-comment
branch
from
041e722
to
f848a2d
Compare
| - name: Generate composer diff | ||
| if: ${{ steps.composer-lock-changed.outputs.any_changed == 'true' }} | ||
| id: composer-diff | ||
| uses: IonBazan/composer-diff-action@v1 | ||
| with: | ||
| with-platform: true | ||
| with-links: true |
There was a problem hiding this comment.
Does it make sense to also include the underlying CLI command in ddev via a Dockerfile? That way, developers are one step closer to debugging things when they go wrong.
There was a problem hiding this comment.
I don't really think so, and I would actually advocate for removing composer-lock-diff as a task/command. I never found a need for it to run this locally in the same way that it does with the action. I could just always debug the action itself or report an issue upstream with the community action.
It is worth noting this is a change from using https://github.com/davidrjonas/composer-lock-diff to https://github.com/IonBazan/composer-diff. There are more benefits to this change relating to maintained code and Github Actions support:
| composer-lock-diff | composer-diff |
|---|---|
Run composer-lock-diff |
Run composer diff (composer plugin) |
| Last release Mar 2022 | Last release Apr 2024 |
| No GitHub action available | Maintained GitHub action available |
| Supports Drupal.org diffs | Supports Drupal.org diffs now (see IonBazan/composer-diff#24) |
That said, having just the composer diff command available from the Dockerfile was useful, but I don't think it needs to be a downloaded binary and task at all since it would be available as a native composer command.
|
@beto-aveiga is going to review this for #318 (comment) and if there's overlap file a PR against this branch. |
|
We discussed this in the drainpipe sync today. I'd kinda like to see the performance improvement (to not use ddev) and the functional improvement of posting NOT in the description be separate PRs. [Lullabot internal link to a sample action.] |
|
@beto-aveiga is going to code review and test this branch out on an existing project to determine time saved. |
| - uses: actions/cache@v4 | ||
| - name: Check if composer.lock was changed | ||
| id: composer-lock-changed | ||
| uses: tj-actions/changed-files@v44 |
There was a problem hiding this comment.
@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.
| - name: Install and Start DDEV | ||
| uses: ./.github/actions/drainpipe/ddev | ||
| - name: Delete sticky pull request comment | ||
| uses: marocchino/sticky-pull-request-comment@v2 |
There was a problem hiding this comment.
@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.
| - name: Generate composer diff | ||
| if: ${{ steps.composer-lock-changed.outputs.any_changed == 'true' }} | ||
| id: composer-diff | ||
| uses: IonBazan/composer-diff-action@v1 |
There was a problem hiding this comment.
@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.
| -d @processed.json | ||
| - name: Post sticky pull request comment | ||
| if: ${{ steps.composer-lock-changed.outputs.any_changed == 'true' }} | ||
| uses: marocchino/sticky-pull-request-comment@v2 |
There was a problem hiding this comment.
@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.
There was a problem hiding this comment.
How do we get notified of new releases to update the hash to? Monitor the log files for deprecation notices?
There was a problem hiding this comment.
While I agree that locking it down to commit hashes is better for security, the problem becomes when we need to update the workflows to all the consumers, or any repositories haven't been updated in a while, their actions could potentially break due to GitHub changes. It's a trade-off that we can balance with only doing it for very specific, trusted actions.
|
@beto-aveiga Instead of pulling this out of DDEV, we discussed bundling it in with the Static Tests which does use DDEV but we're no longer having to wait for another job to build that container since it's already available. |
|
I'm not sure that bundling it is a wise idea, if static tests fail, or DDEV fails to spin up, we run into the same problem where PRs won't have their diffs posted, which can still be very helpful information to review on a PR while broken jobs get resolved. |
Fixes #332
Improvements:
Testing PR:
Lullabot/drainpipe-test#11
Run compares: