-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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: add metamaskbot codeowner announcement #30668
base: main
Are you sure you want to change the base?
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
Builds ready [ee3811e]
Page Load Metrics (1564 ± 41 ms)
Bundle size diffs
|
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 have not actually tested functionality, do you have a PR open against this that tests it?
package.json
Outdated
@@ -120,6 +120,7 @@ | |||
"check-pr-has-required-labels": "ts-node ./.github/scripts/check-pr-has-required-labels.ts", | |||
"close-release-bug-report-issue": "ts-node ./.github/scripts/close-release-bug-report-issue.ts", | |||
"check-template-and-add-labels": "ts-node ./.github/scripts/check-template-and-add-labels.ts", | |||
"identify-codeowners": "ts-node ./.github/scripts/identify-codeowners.ts", |
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.
Two different comments on this line:
- We are trying to migrate away from
ts-node
and totsx
. And then a little later we can probably go to Node 20+ native TypeScript support. - I don't think commands that can run only on CI should be in
package.json
, I think it just adds to the clutter.
(These two comments also probably apply to some of the nearby lines)
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.
What pattern would you suggest I follow?
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'd say remove this line from package.json
and make the yml execute yarn tsx ./.github/scripts/identify-codeowners.ts
I have not investigated, but if any of the nearby lines fall in the same boat, do the same thing.
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.
Done here: f5e6e1b. Mind if we take care of the other ones in a separate PR?
package.json
Outdated
@@ -374,6 +375,7 @@ | |||
"lottie-web": "^5.12.2", | |||
"luxon": "^3.2.1", | |||
"nanoid": "^3.3.8", | |||
"picomatch": "^4.0.2", |
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.
How important is using picomatch
, and how much more code is it to use a package we already use?
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.
It's needed, the library is also pretty lightweight. I don't think we're doing any glob pattern matching anywhere.
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 was trying to understand this... I guess it's because CODEOWNERS is written with globs?
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.
Yessir
- name: Checkout repository | ||
uses: actions/checkout@v4 | ||
|
||
- name: Setup environment | ||
uses: metamask/github-tools/.github/actions/setup-environment@1d657e262aea7e3f216754febb624831527d2565 |
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 is going to have not a direct merge conflict, but an indirect merge conflict with #29979, so let's watch out for which one merges first.
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.
Will do 🫡
Yeah, I was testing against #30669 |
Okay I tried it out, pretty cool.
|
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
Builds ready [175396c]
Page Load Metrics (1844 ± 57 ms)
Bundle size diffs
|
Builds ready [65ccd4c]
Page Load Metrics (1865 ± 93 ms)
Bundle size diffs
|
Builds ready [894a904]
Page Load Metrics (1655 ± 95 ms)
Bundle size diffs
|
Builds ready [9e6ed13]
Page Load Metrics (1685 ± 61 ms)
Bundle size diffs
|
Builds ready [e02a949]
Page Load Metrics (1865 ± 61 ms)
Bundle size diffs
|
Builds ready [cdad138]
Page Load Metrics (1830 ± 95 ms)
Bundle size diffs
|
Builds ready [3ebad9d]
Page Load Metrics (2180 ± 101 ms)
Bundle size diffs
|
Description
Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions:
Manual testing steps
Screenshots/Recordings
After
Pre-merge author checklist
Pre-merge reviewer checklist