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

Update ComposerLockDiff workflow to run without DDEV and use a sticky pull request comment #538

Closed
wants to merge 17 commits into from
Closed

Update ComposerLockDiff workflow to run without DDEV and use a sticky pull request comment #538

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5a7f642
Update ComposerLockDiff workflow to run natively and use a sticky pul…
davereid Apr 22, 2024
f49cd63
Make workflow callable and test it in the PR itself.
davereid Apr 22, 2024
a256f1b
Ensure pull request comment runs if composer.lock was changed.
davereid Apr 22, 2024
7fcc0c1
Added test workflow.
davereid Apr 22, 2024
5c9a084
Flip around callable workflows.
davereid Apr 22, 2024
4d4433a
Update type to number.
davereid Apr 22, 2024
f848a2d
Add composer lock for testing.
davereid Apr 22, 2024
52b4582
Merge branch 'main' into composer-lock-diff-sticky-pr-comment
davereid Apr 24, 2024
f2c8724
Just use a direct workflow for now.
davereid Apr 24, 2024
57d94dd
Removing composer lock file.
davereid Apr 24, 2024
e273b0e
Fix fetch-depth on checkout.
davereid Apr 24, 2024
6ab1ccc
actions/cache step no longer needed without DDEV
davereid Apr 24, 2024
5f9f36a
Updated README about Composer Lock Diff and what it does.
davereid Apr 24, 2024
344933b
Merge branch 'main' into composer-lock-diff-sticky-pr-comment
davereid Apr 26, 2024
4b3407f
Merge branch 'main' into composer-lock-diff-sticky-pr-comment
davereid May 1, 2024
fbfe18d
Merge branch 'main' into composer-lock-diff-sticky-pr-comment
davereid May 7, 2024
d436532
Merge branch 'main' into composer-lock-diff-sticky-pr-comment
mrdavidburns Jul 16, 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
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,8 @@ Tests can be run locally with [act](https://github.com/nektos/act):
`act -P ubuntu-latest=ghcr.io/catthehacker/ubuntu:runner-latest -j Static-Tests`

### Composer Lock Diff
Update Pull Request descriptions with a markdown table of any changes detected
in `composer.lock` using [composer-lock-diff](https://github.com/davidrjonas/composer-lock-diff).
Posts a comment in Pull Requests with a markdown table of any changes detected
in `composer.lock` using [composer-diff](https://github.com/IonBazan/composer-diff).

```json
"extra": {
Expand Down
61 changes: 24 additions & 37 deletions scaffold/github/workflows/ComposerLockDiff.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,46 +17,33 @@ jobs:
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 2
fetch-depth: 0

- uses: actions/cache@v4
- name: Check if composer.lock was changed
id: composer-lock-changed
uses: tj-actions/changed-files@v44
Copy link
Member

Choose a reason for hiding this comment

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

@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.

with:
path: ${{ github.workspace }}/.ddev/.drainpipe-composer-cache
key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }}
restore-keys: |
${{ runner.os }}-composer-
files: 'composer.lock'

- uses: ./.github/actions/drainpipe/set-env

- name: Install and Start DDEV
uses: ./.github/actions/drainpipe/ddev
- name: Delete sticky pull request comment
uses: marocchino/sticky-pull-request-comment@v2
Copy link
Member

Choose a reason for hiding this comment

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

@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.

with:
git-name: Drainpipe Bot
git-email: no-reply@example.com
ssh-private-key: ${{ secrets.SSH_PRIVATE_KEY }}
ssh-known-hosts: ${{ secrets.SSH_KNOWN_HOSTS }}

- name: Build Project
run: ddev composer install

- name: Install composer lock diff
run: ddev composer global require davidrjonas/composer-lock-diff:^1.0
header: composer-lock-diff
delete: true

- name: Prepare repository
run: |
git reset --soft HEAD^1
git reset .
- name: Generate composer diff
if: ${{ steps.composer-lock-changed.outputs.any_changed == 'true' }}
id: composer-diff
uses: IonBazan/composer-diff-action@v1
Copy link
Member

Choose a reason for hiding this comment

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

@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.

with:
with-platform: true
with-links: true
Comment on lines +34 to +40
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@davereid davereid Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


- name: Run composer lock diff
run: |
curl -f \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}" \
https://api.github.com/repos/${{ github.repository }}/pulls/$DRAINPIPE_PR_NUMBER | jq '. | {body}' > pull_request.json
ddev task "github:composer-lock-diff pull_request='pull_request.json' json_file='processed.json'"
curl -f \
-X PATCH \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}" \
https://api.github.com/repos/${{ github.repository }}/pulls/$DRAINPIPE_PR_NUMBER \
-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
Copy link
Member

Choose a reason for hiding this comment

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

@deviantintegral suggests that we pin to a commit hash so future releases don't unexpectedly break our builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we get notified of new releases to update the hash to? Monitor the log files for deprecation notices?

Copy link
Member Author

@davereid davereid Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

with:
header: composer-lock-diff
message: |
### Composer package changes
${{ (steps.composer-diff.outcome != 'success' && 'Review any changes to composer.lock manually.') || steps.composer-diff.outputs.composer_diff || 'No changes found' }}
Loading