-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: added a shared workflow to auto update browserslist in the MFEs #29
feat: added a shared workflow to auto update browserslist in the MFEs #29
Conversation
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.
Some minor suggestions. Otherwise, looks good to be merged.
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.
Approved, but I'd like more eyes on this first, since it's an org-wide change.
commit-message: 'chore: update browserslist' | ||
title: Update browserslist DB | ||
body: | | ||
Updated browserslist DB |
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 would provide a much more verbose body. Is there a SLA to merging that teams should have? What is the importance of the change? Could we do a short summary of the change? (you can echo things to the Actions environment then use them later in the file, for example this line:
echos variable $PROJECT_ID to the local $GITHUB_ENV, and then it's used in the next step:
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 SLA to merging that teams should have?
I believe we could auto-merge the resulting PR from this workflow. The resulting changes from browserslist --update-db
command update a tiny portion of the package-lock.json file, and is pretty low risk to auto-merge, assuming CI and other status checks pass.
That way, consuming teams won't need to triage/prioritize merging the resulting PR.
commit-message: 'chore: update browserslist' | ||
title: Update browserslist DB | ||
body: | | ||
Updated browserslist DB |
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 SLA to merging that teams should have?
I believe we could auto-merge the resulting PR from this workflow. The resulting changes from browserslist --update-db
command update a tiny portion of the package-lock.json file, and is pretty low risk to auto-merge, assuming CI and other status checks pass.
That way, consuming teams won't need to triage/prioritize merging the resulting PR.
@@ -0,0 +1,7 @@ | |||
{ | |||
"name": "Auto Update Browserslist 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.
nit: "Auto Update Browserslist DB Workflow" maybe?
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.
Similarly, should the file name also include DB, e.g. update-browserslist-db.properties.json
?
run: npm ci | ||
|
||
- name: Update dependencies | ||
run: npx browserslist@latest --update-db |
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.
Note: I just stumbled on https://github.com/c2corg/browserslist-update-action, which is a Github Action we might be able to use here instead. Worth noting that the README for that action mentions:
v1 uses command
npx browserslist@latest --update-db
which is now deprecated and to be removed in a future major browserslist release.
It is being replaced with a standalone NPM package update-browserslist-db
instead.
@abdullahwaheed, sprint check: will you have time to address the reviews in the next couple of weeks? |
label: waiting on author |
…-request-automerge
@adamstankiewicz I have used this action to enable Auto Merge functionality and made suggested changes. Please note that these conditions must met to get PR auto merge. |
remove label: waiting on author |
I see you've requested me for a review. From an Actions perspective this seems fine but I don't know anything about what it's actually doing and am not going to test, so I will not be a reviewer. |
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.
This looks good to me! +1
I approve, but I'm not sure I should be able to merge this. I'll ask around and circle back. |
This also looks good to me! ✅ |
@adamstankiewicz @arbrandes to enable auto merge functionality, |
@abdullahwaheed, I think we should allow repo owners/maintainers to decide. Maybe as part of a checklist on the PR description, checking with the reviewer to agree auto-merging should be enabled for that repo. |
sure, i'll add appropriate comments in the PR description, and i'll also update this shared workflow to make auto merging |
Added a shared Workflow to auto update browserslist