Skip to content

Rework building of Docker images#1382

Merged
amarthadan merged 3 commits into
masterfrom
docker-image-from-released-packages
Aug 26, 2022
Merged

Rework building of Docker images#1382
amarthadan merged 3 commits into
masterfrom
docker-image-from-released-packages

Conversation

@amarthadan

Copy link
Copy Markdown
Contributor

Close #1331

Should be pretty much the final version as far as the Docker containers go but it will be expanded on in #1332 and #1333 so we can publish NPM packages even without building the Docker images.

@amarthadan amarthadan requested a review from a team August 16, 2022 13:18
@amarthadan amarthadan self-assigned this Aug 16, 2022

@dcroote dcroote left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a few quick tests of the dockerhub -dev images for this commit (d0a04df8fdbbdca4b14548256422c0f7d075cf03) and they appeared to be functional 👍

Comment thread .github/workflows/build-test.yml
Comment thread packages/airnode-node/docker/Dockerfile Outdated
Comment thread docker/README.md
This is a Docker container for building all the packages in the monorepo and providing their build artifacts. Other
package specific containers can use this one to obtain files necessary for their run without building or installing any
additional dependencies.
This is a Docker container for building all the packages in the monorepo, publishing the NPM packages (TODO) and

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This is a Docker container for building all the packages in the monorepo, publishing the NPM packages (TODO) and
This is a Docker container for building all the packages in the monorepo, publishing the NPM packages (TODO) and

These TODOs have a tendency to be forgotten, so I'd just avoid them and mention NPM publishing when implemented. Doesn't matter much to me though.

Comment thread docker/README.md
```bash
docker build --build-arg build=local -f docker/Dockerfile -t api3/airnode-artifacts:latest .
# or with no build argument
... -v /var/run/docker.sock:/var/run/docker.sock ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cross platform right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to test it on Mac, it should work there. Not sure about Windows but I don't think being able to build the images there is as important as being able to run them there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, will try to test this today.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we mention (or create an issue) that this is unusable on macOS due to docker copy being unusably slow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the way the files are copied (as we've discussed on Slack). Can you test it again?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that it took so long. I can confirm this works, but I had to add back some missing dependencies.

I have also a bit cleaner suggestion for copying the files. Essentially you can do a git clone from a local repo. See: 47ddc74

Comment thread scripts/packaging/prepare-containers.ts
Comment thread docker/Dockerfile Outdated
Comment thread docker/entrypoint.sh Outdated
Comment thread docker/entrypoint.sh
Comment thread scripts/packaging/prepare-containers.ts
@amarthadan amarthadan force-pushed the docker-image-from-released-packages branch from 0c4b5e6 to 15f9565 Compare August 18, 2022 07:38
@amarthadan amarthadan requested a review from Siegrift August 22, 2022 05:31
Comment thread packages/airnode-node/docker/README.md

@Siegrift Siegrift left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is https://github.com/api3dao/airnode/blob/docker-image-from-released-packages/packages/airnode-examples/src/scripts/rebuild-deployer-container.ts and similar ones for artifacts and client. These are no longer used in the examples and can be removed from package.json (and the test one).

@amarthadan amarthadan force-pushed the docker-image-from-released-packages branch from 15f9565 to c9852f4 Compare August 23, 2022 12:08

@Siegrift Siegrift left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on macOS, left a minor suggestion, but 👍 LGTM.

One thing that threw me off a bit is that the images are tagged as :local by default (previously we used :latest) but I've checked the airnode repo and docs and there are no references to :latest.

Comment thread docker/entrypoint.sh Outdated
Comment thread docker/README.md
```bash
docker build --build-arg build=local -f docker/Dockerfile -t api3/airnode-artifacts:latest .
# or with no build argument
... -v /var/run/docker.sock:/var/run/docker.sock ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that it took so long. I can confirm this works, but I had to add back some missing dependencies.

I have also a bit cleaner suggestion for copying the files. Essentially you can do a git clone from a local repo. See: 47ddc74

@amarthadan amarthadan force-pushed the docker-image-from-released-packages branch from c9852f4 to c4537d3 Compare August 25, 2022 11:30
@amarthadan

Copy link
Copy Markdown
Contributor Author

@Siegrift I've added the dependencies and changed the file copying to follow the .gitignore(s). Can you try it one more time? 🙏

@amarthadan amarthadan requested a review from Siegrift August 25, 2022 11:38
@amarthadan amarthadan force-pushed the docker-image-from-released-packages branch from c4537d3 to 72e41f7 Compare August 26, 2022 09:44
@amarthadan amarthadan merged commit 22aa002 into master Aug 26, 2022
@amarthadan amarthadan deleted the docker-image-from-released-packages branch August 26, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Building Docker images from published NPM packages

3 participants