-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add a workflow for checking and managing built files #8260
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
Conversation
|
Am I right in thinking that when this gets merged, it'll need to have an additional |
Ah, yes! That's correct. I forgot to make that change in this PR since I copy/pasted from my fork where I needed this to run for testing purposes. |
joemcgill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea. Using it to ensure changes to built files are added to PRs helps speed up the committer workflow as well. I could see us extend this to all PRs and not just the automated ones eventually.
Optionally, could we use this same workflow to cause a PR from a fork to fail a commit check if there are uncommitted changes so the contributor (and code reviewer) sees that feedback?
| - name: Stage changes | ||
| if: ${{ steps.built-file-check.outputs.uncommitted_changes != '' }} | ||
| run: git add . | ||
|
|
||
| - name: Commit changes | ||
| if: ${{ steps.built-file-check.outputs.uncommitted_changes != '' }} | ||
| run: | | ||
| git commit -m "Automation: Updating built files with changes. [dependabot skip]" | ||
|
|
||
| - name: Push changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to separate these steps or could they be combined into one step. Also, if something were to fail at this point, can we provide good feedback in the workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any strong preference here, but thought splitting the commands into individual steps would make it more clear where a failure occurred.
What type of feedback would you like to see? Right now, it will display the full error message as returned by Git.
…-linter-fixes Add/workflow for built files linter fixes
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I've gone and created a GitHub App and configured it as required for this to work. Let's give it a try! |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…uilt-files # Conflicts: # .github/workflows/check-built-files.yml # .github/workflows/reusable-check-built-files.yml
|
Hmm, there's something different between my fork where I tested and wordpress-develop. I've configured everything the same way, but the token is unable to checkout the repository and I'm not sure why. I've gone and disabled the workflow until someone is able to figure out why. |
|
So this is an issue with PRs coming from forks, which I couldn't test in my own fork. There are two issues:
Adjusting the inputs for the The only unknown here is whether the GitHub App will be able to act in the same manner as maintainers, given the PR author has checked "Allow edits and access to secrets by maintainers". |
|
Closing out. This is committed and seems to be working well. Will monitor and open separate PRs if necessary. |
This adds a workflow that automatically commits missing changes to versioned files managed through the build process when they are missed.
This was created primarily to help ensure necessary changes from Dependabot PRs are included so that various workflows pass, but it also helps contributors make sure they are not forgetting something.
The use of a GitHub App is required because of how permissions are limited for Dependabot PRs. Pushing to a branch created by Dependabot (or another fork) without the use of an app results in no GitHub Actions being triggered and defeats the purpose.
I have been testing this on my fork where Dependabot is currently configured to monitor all direct npm dependencies. View any Dependabot PR to see this workflow committing back changes to built files: https://github.com/desrosj/wordpress-develop/pulls
Trac ticket: https://core.trac.wordpress.org/ticket/62221
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.