-
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
Enable building Docker images for released packages #1495
Conversation
c9e282a
to
f8ae0a6
Compare
1f87e4a
to
a7c35f2
Compare
f8ae0a6
to
2fb32d7
Compare
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 tested this by building the images locally and testing them. I wanted to also use the local NPM packages but the current setup won't work on macOS due to this.
I tried publishing ports for both docker:scripts:npm-registry
command and also to the verdaccio container run inside the docker, but it didn't help.
Otherwise code 👍 LGTM
|
||
## How it works | ||
- Start/stop a local NPM registry Docker container | ||
- Build and publish NPM packages to both local and official (TODO) NPM registry |
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.
intended 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.
Yes, that one is intended, I'm working on that right now 🙂
package.json
Outdated
"docker:build": "yarn docker:build:packaging && yarn docker:build:images", | ||
"docker:build:images": "docker run --rm -v $(pwd):/airnode -v /var/run/docker.sock:/var/run/docker.sock api3/airnode-packaging:latest", | ||
"docker:build:packaging": "docker build --tag api3/airnode-packaging:latest --file docker/Dockerfile docker/", | ||
"docker:build:local": "yarn docker:build:packaging && yarn docker:build:images:local", |
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: can you sort them alphabetically?
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 swear they were but I changed them so many times they got out of sync 😅 Will do.
MacOS's lack of containers is... sad. I know it's generally slower due to the FS mounts but I didn't realise it also had this network limitation, it makes sense. |
Actually, the fix is simple (just publish the ports) and together with @amarthadan we already fixed the issue. |
2fb32d7
to
ff4744b
Compare
ff4744b
to
2d4714a
Compare
"docker:scripts:build-docker-images": "docker run --rm -v /var/run/docker.sock:/var/run/docker.sock api3/airnode-packaging:latest build-docker-images", | ||
"docker:scripts:build-docker-images:latest": "yarn docker:scripts:build-docker-images", | ||
"docker:scripts:build-docker-images:local": "yarn docker:scripts:build-docker-images --npm-registry local --npm-tag local --docker-tag local", | ||
"docker:scripts:npm-registry": "docker run --rm -v /var/run/docker.sock:/var/run/docker.sock api3/airnode-packaging:latest npm-registry", |
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: This always runs with the latest
tag, but other script can be run with local
and latest` tags.
docker/scripts/npm-registry.ts
Outdated
@@ -38,7 +38,9 @@ export const stopNpmRegistry = () => { | |||
|
|||
export const startNpmRegistry = async () => { | |||
const npmRegistryContainerName = `airnode-npm-registry-${randomBytes(4).toString('hex')}`; | |||
runCommand(`docker run -d --rm --name ${npmRegistryContainerName} ${NPM_REGISTRY_CONTAINER_IMAGE}`); | |||
runCommand( | |||
`docker run -d --rm -p 127.0.0.1:${NPM_REGISTRY_CONTIANER_PORT}:${NPM_REGISTRY_CONTIANER_PORT} --name ${npmRegistryContainerName} ${NPM_REGISTRY_CONTAINER_IMAGE}` |
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.
change NPM_REGISTRY_CONTIANER_PORT
to NPM_REGISTRY_CONTAINER_PORT
2d4714a
to
6f1ae19
Compare
Also cleanup and modularization of packaging scripts.
6f1ae19
to
d2f3fca
Compare
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
Not really solving #1332 per se but rather a forgotten part of #1331. Also, some refactoring, clean up, and breaking into smaller parts that we can now build on 🙂