-
Notifications
You must be signed in to change notification settings - Fork 6
Update WordPress JS dependencies #174
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
base: main
Are you sure you want to change the base?
Conversation
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
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.
Thank you for adding this workflow! I left a couple of minor suggestions and comments.
Also, I took the liberty to push a rather major rewrite of the documentation; putting this into line suggestions would have been impossible.
Please check my comments and the updated documentation. Thank you!
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
…r.yml Co-authored-by: Philipp Bammes <[email protected]> Signed-off-by: Luis Rosales <[email protected]>
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
.github/workflows/update-wordpress-js-dependencies-orchestrator.yml
Outdated
Show resolved
Hide resolved
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.
Thank you for incorporating all the changes, and I apologize for the number of new comments. Most are minor, as they are about documentation wording alignment or previously overlooked suggestions, but there's one regarding the documentation that does not reflect the move from repository_dispatch
to workflow_dispatch
.
This should be the last round. 🤞🏽
PACKAGE_MANAGER: npm | ||
GITHUB_USER_EMAIL: ${{ secrets.GITHUB_USER_EMAIL }} | ||
GITHUB_USER_NAME: ${{ secrets.GITHUB_USER_NAME }} | ||
GITHUB_USER_SSH_KEY: ${{ secrets.GITHUB_USER_SSH_KEY }} | ||
GITHUB_USER_SSH_PUBLIC_KEY: ${{ secrets.GITHUB_USER_SSH_PUBLIC_KEY }} | ||
WP_DIST_TAG: ${{ inputs.WP_DIST_TAG }} | ||
NODE_AUTH_TOKEN: ${{ secrets.NPM_REGISTRY_TOKEN }} | ||
NPM_REGISTRY_DOMAIN: ${{ inputs.NPM_REGISTRY_DOMAIN }} |
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.
In the meantime, #180 has been merged. Please validate which global env variables are still needed here and which can be moved to step level.
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 left only non secrets
- name: Set up Git | ||
env: | ||
GITHUB_USER_EMAIL: ${{ secrets.GITHUB_USER_EMAIL }} | ||
GITHUB_USER_NAME: ${{ secrets.GITHUB_USER_NAME }} |
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.
The latest change made me realize that an if
check is missing here because these credentials are optional.
GITHUB_USER_NAME: ${{ secrets.GITHUB_USER_NAME }} | |
GITHUB_USER_NAME: ${{ secrets.GITHUB_USER_NAME }} | |
if: ${{ env.GITHUB_USER_EMAIL != '' && env.GITHUB_USER_NAME != '' }} |
NPM_REGISTRY_DOMAIN: ${{ inputs.NPM_REGISTRY_DOMAIN }} | ||
NODE_AUTH_TOKEN: ${{ secrets.NPM_REGISTRY_TOKEN }} |
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.
Have you verified that NPM_REGISTRY_DOMAIN
and NODE_AUTH_TOKEN
are indeed required again here and in the "Update dependencies" step below? I would expect this to be similar to Composer's COMPOSER_AUTH
variable, which is only required once.
4b2c3d7
to
a810bb3
Compare
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature
What is the current behavior? (You can also link to an open issue here)
There is no workflow to automatically create a PR with updated WordPress dependencies.
What is the new behavior (if this is a feature change)?
It adds two new workflows to create a PR with updated WordPress dependencies.
The reusable workflow for the orchestrator was added in here:
reusable-workflows/.github/workflows/update-wordpress-js-dependencies-orchestrator.yml at update_wordpress_js_dependencies · inpsyde/reusable-workflows
How does it work?
Individual package
The workflow can be called from the individual package or from the orchestrator.
Example:
Orchestrator
An orchestrator is a repository that consumes other packages, typically a website repository.
Orchestrator example:
The workflow is triggered on demand. The user needs to define the wp dist tag and could optionally define some repos in the
PACKAGES
input in a comma-separated string.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No
Other information: