-
Notifications
You must be signed in to change notification settings - Fork 72
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
Automate releasing of official packages #1538
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.
Btw. have you also looked at https://github.com/changesets/action ? It does the PR opening and potentially NPM package releasing. I'm sorry for not giving the link sooner, but it just now came to my mind that I saw this thing being mentioned in the changeset docs...
I am fine with our own implementation though...
docker/scripts/git.ts
Outdated
@@ -0,0 +1,21 @@ | |||
import { runCommand } from './utils'; |
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 like this module.
docker/README.md
Outdated
docker run --rm -v $(pwd):/airnode -v /var/run/docker.sock:/var/run/docker.sock api3/airnode-packaging:latest npm pull-request --release-version 0.11.0 --head-branch v0.11 --base-branch master | ||
``` | ||
|
||
<!-- TODO |
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 guess this will be resolved in further commits...
docker/README.md
Outdated
Example: | ||
|
||
```bash | ||
docker run --rm -v $(pwd):/airnode -v /var/run/docker.sock:/var/run/docker.sock api3/airnode-packaging:latest npm pull-request --release-version 0.11.0 --head-branch v0.11 --base-branch master |
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.
So I first checkout the new-release
branch, then push it and then create a release from it to the master, right?
docker/README.md
Outdated
Example: | ||
|
||
```bash | ||
docker run --rm -v $(pwd):/airnode -v /var/run/docker.sock:/var/run/docker.sock api3/airnode-packaging:latest npm publish --npm-tags latest,v0.11 |
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 do I specify which release branch I want to release the NPM packages?
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.
Or the point is to create a release PR, then check out the branch and run this docker command to release the packages from it? This could be explained somehwere.
@@ -118,10 +118,16 @@ yargs(process.argv.slice(2)) | |||
default: ['latest'], | |||
type: 'array', | |||
}, | |||
'release-branch': { |
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.
👍 yeah, I felt like this was missing in the previous commits.
docker/scripts/utils.ts
Outdated
// There are currently 2 secrets being logged: | ||
// * DockerHub auth token - stdin | ||
// * GitHub auth token - remote URL | ||
// Third secret, the NPM auth token, is stored in a file therefor not logged |
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.
therefrE
Did a shallow pass only today, will take a proper look tomorrow. |
No worries, I knew about it 🙂 I decided against it mainly because it's just a CI action and I wanted a solution you can run locally as well. I could have used some of the code though... |
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.
👍 LGTM,
I'd love to see this in action though. Maybe we could do a test release?
|
||
const prePublishSetup = async (npmRegistry: string, releaseBranch?: string) => { | ||
const npmRegistryUrl = getNpmRegistryUrl(npmRegistry); | ||
fetchProject(!releaseBranch); |
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: fetch suggests that it's asynchronous
runCommand(`yarn changeset publish --no-git-tag --snapshot --tag ${npmTag}`, { cwd: '/build', stdio: 'ignore' }); | ||
|
||
git.setIdentity('API3 Automation', '[email protected]'); | ||
github.authenticate(); |
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 strange that this is sync. Maybe rename authenticateSync
?
I'd leave it for v0.10. I don't really want to mess up the official API3 registry with random versions. |
9c07bc4
to
c1dcae4
Compare
Close #1333
Review commit by commit.