api3dao / api3-dao-dashboard

API3 DAO dashboard
api3.eth/
14 stars 14 forks source link

Pin production branch Docker parent image to node:16-alpine #353

Closed dcroote closed 1 year ago

dcroote commented 1 year ago

Fixes #352.

dcroote commented 1 year ago

People will only look on the instructions in the main branch anyway.

Then we should have this in both main and production- the main branch README actually instructs the user to pull the production branch: git clone --depth=1 --branch production https://github.com/api3dao/api3-dao-dashboard.git so that they can interact with the DAO dashboard directly without needing to trust an intermediary gateway service

dcroote commented 1 year ago

@Siegrift I applied the #328 fix to this production branch PR as requested, but I don't have write access so can you merge?

Siegrift commented 1 year ago

Then we should have this in both main and production- the main branch README actually instructs the user to pull the production branch

Yeah, good point. However, the old docker image wanted to build things from the local folder which breaks on Windows. The #328 changed this to pull the repo inside the docker and build/run it there. So I think we should merge it with the updated Dockerfile and also the README instructions. WDYT?

dcroote commented 1 year ago

changed this to pull the repo inside the docker and build/run it there

😕 I see how this enables it to work on Windows, but pulling from the repo instead of building from local is incompatible with building the image in a CI workflow (i.e. I make a modification, open a PR, but the PR build will just pull from the repo). Ultimately I don't think we should be asking users to build the images, but instead should build and publish them (DAO-161). This would also avoid the Windows path issues (which I confirmed still exist) as Windows users could run the prebuilt image. Having a CI workflow for Docker builds (on push, PR, and even cron schedule) would also allow us to catch issues like this where the build becomes broken.

I would propose adding a caveat to the production branch README saying it cannot be built on Windows (instead use WSL2, which I confirm works) and then adding the CI Docker build workflow for the production branch. I can volunteer for this if it helps.

Siegrift commented 1 year ago

I see how this enables it to work on Windows, but pulling from the repo instead of building from local is incompatible with building the image in a CI workflow

Yeah, but the current way is "running from source" and for that, you always want to get the content of the production branch. And I understand you can't build a docker image locally out of your branch, but that is not necessary.

Ultimately I don't think we should be asking users to build the images, but instead should build and publish them (DAO-161). This would also avoid the Windows path issues (which I confirmed still exist) as Windows users could run the prebuilt image. Having a CI workflow for Docker builds (on push, PR, and even cron schedule) would also allow us to catch issues like this where the build becomes broken.

I think so as well, afaik @bbenligiray wasn't against this either.

I can volunteer for this if it helps.

That would be great. I added you as a maintainer to the repo (fyi: @bbenligiray). I am also curious about what @mcoetzee thinks about this.

mcoetzee commented 1 year ago

I.m.o. it would be great to give the option of either using a published Docker image, or to build the image yourself locally.

When to build and publish Sounds like we would only want to publish when building for the production branch.

When to just build (to catch issues early) I would imagine that building for main (and not every PR) would allow us to catch issues early enough?

Siegrift commented 1 year ago

I.m.o. it would be great to give the option of either using a published Docker image, or to build the image yourself locally

Yeah, on second thought, building from the source is nice and if we would publish as part of CI, we would want to build the image from the source anyway... However, building docker from source for the dev branch would be done mostly by developers or advanced users wanting to try unreleased features and they can just do that without docker.

Also, building from source will work only on unix-like systems so we should have a note about that in the readme.

I would imagine that building for main (and not every PR) would allow us to catch issues early enough?

Yeah, building the image is fine. If the base branch is production, the CI should publish the image to Dockerhub. For that, we need some versioning system though.

dcroote commented 1 year ago

Thank you @Siegrift and @mcoetzee! I think we're aligned:

I've completed the third bullet in the latest commit on this PR, so with your approval @Siegrift I'll merge and start on DAO-228.