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

Replace ComposerLockDiff functionality with a sticky PR comment instead #332

Closed
2 tasks
justafish opened this issue Nov 2, 2023 · 7 comments
Closed
2 tasks
Assignees
Labels
bug Something isn't working

Comments

@justafish
Copy link
Member

justafish commented Nov 2, 2023

Trying to preserve the original content of a comment is causing too many bugs, and doesn't work well when there are edit conflicts

@davereid
Copy link
Member

davereid commented Jan 8, 2024

Related: #317

@davereid
Copy link
Member

davereid commented Feb 1, 2024

My proposal (with approval already from @justafish) is to replace updating the PR body with a sticky comment using https://github.com/marocchino/sticky-pull-request-comment.

@mrdavidburns
Copy link
Member

@YesCT and I were just talking about this yesterday. We've had multiple projects where we successfully switched to posting to a comment to avoid this issue. +1 for switching Drainpipe posting to comment.

@davereid
Copy link
Member

davereid commented Feb 1, 2024

Also would be looking to remove DDEV as a dependency of this task to help speed up the job (would also fix #239) and just use composer natively like we did on another client independent of Drainpipe which seemed to work really well and be fast.

@deviantintegral
Copy link
Member

I updated this to also add a one-line change to only run the action at all if composer.lock is modified.

@davereid
Copy link
Member

davereid commented Feb 22, 2024

I think it would still need to run on push because it's possible for a PR to have a composer.lock change initially, but then not have one after merging in updates, so it would still need to delete the comment in that case. But running the diff could be conditional on changes in that file.

@davereid davereid changed the title Replace ComposerLockDiff functionality with a code annotation instead Replace ComposerLockDiff functionality with a sticky PR comment instead Mar 13, 2024
@davereid davereid self-assigned this Apr 16, 2024
@davereid davereid added this to the Composer Lock Diff milestone May 22, 2024
@justafish
Copy link
Member Author

here's an alternative solution that combines it with the security checks - 2 birds with 1 stone!
#636

@justafish justafish self-assigned this Aug 13, 2024
justafish added a commit that referenced this issue Aug 14, 2024
…iff (#636)

* Issue #112 #332: Seperate Security Checks and Replace Composer Lock Diff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants