-
-
Notifications
You must be signed in to change notification settings - Fork 529
fix: move to trusted publishing #1551
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
Also moves the CI workflow to npm and updates CI in general. Moves the release workflow to a separate one that is called when we want to do a release. This manual action is in line with how we do `@sequelize/*` releases, I have removed the tests from it since tests need to pass before we merge.
| - v6 | ||
| - v7 | ||
| pull_request: | ||
| on: pull_request |
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.
why does this migrate from on push to on pull_request?
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.
We don't run it 'on push' at the regular repo, so this makes it more consistent. Reasoning there was because we do everything with PRs that are up to date, we don't need to run it again after merging
| - run: docker compose up -d ${DIALECT} | ||
| - run: docker run --link ${DIALECT}:db --net cli_default jwilder/dockerize -wait tcp://${DIALECT}:${SEQ_PORT::-1} -timeout 2m | ||
| - run: yarn test | ||
| - run: npm test |
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.
while i dont mind, why are we moving to npm=
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.
Because yarn doesn't offer the trusted publishing (yet). So we're locking in to the npm registry and its package manager
| RELEASE_NOTES: ${{ steps.release.outputs.new_release_notes }} | ||
| PACKAGE_NAME: sequelize-cli | ||
| run: | | ||
| curl -X POST "https://api.opencollective.com/graphql/v2" \ |
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.
does this mean we dont need the dispatch job anymore that i created a few years ago?
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.
Yup!
| run: | | ||
| curl -X POST "https://api.opencollective.com/graphql/v2" \ | ||
| -H "Content-Type: application/json" \ | ||
| # TODO: use OAuth instead of Personal-Token so we can create the updates from the organization instead of a user |
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 about this?
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.
Now when we post something to OpenCollective it's done under your name. With OAuth we should be able to do it under the name of the organization. This is a TODO for a later PR
| "*.js": "eslint" | ||
| }, | ||
| "release": { | ||
| "plugins": [ |
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.
you are really removing a lot :D why is that?
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 the default config already, so this cleans it up
Pull Request check-list
Please make sure to review and check all of these items:
npm run testpass with this change (including linting)?Description of change
This PR moves our release flow to use trusted publishing. Also moves the CI workflow to npm and updates CI in general. Moves the release workflow to a separate one that is called when we want to do a release. This manual action is in line with how we do
@sequelize/*releases, I have removed the tests from it since tests need to pass before we merge.About the changes to notifications; twitter was not working so I've removed that. The special OpenCollective action was nice, but did a lot that I feel like we can just include it directly (especially with v7 being a monorepo so we only define it once there)
Marked as a fix so it will publish a new release