-
Notifications
You must be signed in to change notification settings - Fork 84
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
fix: resolve CI issues #163
Conversation
naorpeled
commented
Dec 31, 2024
•
edited
Loading
edited
- added myself as maintainer
- resolved CI issues
- dropped Node 14 and 16 official support
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 LGTM but I don't think it will resolve the CI issues
Oh latest changes will fix it. |
forgot to push the actual fix :P |
@jeremydaly you decide if you want this as one thing, or if you want to do #162 first then do #163 on top for the readme changes. I have no preference |
on: | ||
pull_request: | ||
types: [opened, reopened, synchronize] | ||
push: |
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.
Personally I think it's nice to keep this on push because I like having the status in the commit history
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.
Makes sense, adding it back now :)
2a91672
to
90d1f6a
Compare
Co-authored-by: Ben Herila <[email protected]>
90d1f6a
to
f9fc8af
Compare
@bherila addressed your comments, WDYT? |
I see no problems with anything in here. I defer to @jeremydaly however :D |