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

Backport "pull request automation use regular npm install" #66447

Merged

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Oct 25, 2024

What?

Backport of #66314.

Why?

Because CI is failing npm install on the wp/6.7 branch.

See discussion in WP Slack https://wordpress.slack.com/archives/C02QB2JS7/p1729680379803689.

How?

Manual backport.

Testing Instructions

Deferring to @sirreal to double check this makes sense.

Testing Instructions for Keyboard

Screenshots or screencast

Remove single-package installation from this workflow that creates problems
when migrating to npm workspaces.

The regular npm install workflow is frequently available in cache.

---

Co-authored-by: sirreal <[email protected]>
Co-authored-by: gziolo <[email protected]>
# Conflicts:
#	.github/workflows/pull-request-automation.yml
@getdave getdave added the [Type] Enhancement A suggestion for improvement. label Oct 25, 2024
@getdave getdave requested a review from desrosj as a code owner October 25, 2024 08:14
Copy link

github-actions bot commented Oct 25, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sirreal <[email protected]>
Co-authored-by: kevin940726 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

This looks correct to me 👍

Other release branches may experience the same issue.

This workflow was fragile because it was effectively configured on one commit in a way that relied on the state of the repo in another commit. This change should make it less fragile.

@sirreal
Copy link
Member

sirreal commented Oct 25, 2024

Strange. That error on CI is exactly what was observed previously that was fixed by the same change in #66314.

I'll investigate soon…

@kevin940726
Copy link
Member

The failing workflow is running on the pull_request_target event.

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does.

That is, the workflow is still the old one on wp/6.7, not the updated one in this PR. I think the failure is expected. We might just have to merge this to see if it works.

@sirreal
Copy link
Member

sirreal commented Oct 25, 2024

Good spot @kevin940726, that would explain why this fix didn't work on the branch 🙂

I maintain that this change should be merged to wp/6.7 to fix the issue 👍

@getdave getdave merged commit 583561f into wp/6.7 Oct 25, 2024
69 of 70 checks passed
@getdave getdave deleted the backport/pull-request-automation-use-regular-npm-install branch October 25, 2024 09:49
@sirreal
Copy link
Member

sirreal commented Oct 25, 2024

This was tested with #66457 and it seemed to work correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants