umbrel: Set to v1.0.5.

[JoeGruffins]

Update the docker file to get a certain version of dcrdex from github rather than using the local repo so that we can update the docker file itself without updating the tag.

[JoeGruffins]

Also removed the Dockerfile.release file because it seems redundant. Do we need it?

[peterzen]

LGTM. Dockerfile.release is not used elsewhere at present AFAIK; safe to remove.

[JustinBeBoy]

LGTM

[JustinBeBoy]

I think we should validate the version before cloning it.

git ls-remote --refs --tags https://github.com/decred/dcrdex.git | grep -q "refs/tags/$VERSION" || \
    { echo "ERROR: Version $VERSION does not exist"; exit 1; } && \
    git clone --depth 1 --branch $VERSION https://github.com/decred/dcrdex.git

[peterzen]

Unnecessary complexity IMO, docker build will bail out if git clone fails

[peterzen]

On a second thought, I'm not sure that changing the Dockerfile this way is an entirely good idea. The --build-arg/ git clone vs COPY makes it difficult to work/debug build issues on the current clone. This workflow can be triggered multiple times manually in GH, in case the build fails intermittently the action can be triggered again.

[JoeGruffins]

makes it difficult to work/debug build issues on the current clone.

Do you mean using docker for building master?

[peterzen]

Do you mean using docker for building master?

More like when you're debugging something related to the dockerized version or related to the build itself, and you want to build an image locally off of the current clone. Works OK in production in GH workflows, though the additional git clone slows things down. The dcrd docker image builds the same way (I guess this PR borrows from that Dockerfile) but this adds complexity that's not really necessary here.

There is also another element I just recalled - Umbrel images are expected to be able to build in an offline environment in order to be eligible to be added to the official app store (as a safeguard against supply chain attacks) - so no network requests within the Dockerfile. This does not affect Bisonwallet at present in the context of the community app store but essentially disqualifies the app to pass the eligibility test, should we ever want to apply for an official app store entry.

Why did you feel this change was required?

[JoeGruffins]

I did not look at dcrd, this just seems to be what's best.

Why did you feel this change was required?

1. We can't update the docker file without updating the tag if we have to checkout the tag first. This makes things very awkward.
2. Currently it copies over the user's entire dcrdex folder, which may have sensitive information related to testing, like providers json files that have an api secret. This is also copying the entire local .git history? Probably fine since the instructions are to clone a new repo first.
3. When researching almost all suggestions were to get the tag from git if we want to build at the tag.

so no network requests within the Dockerfile

I did not know this part. I'm a little confused though aren't all the docker parts downloaded? We can include a sha comparison but I guess that wouldn't matter?

[JoeGruffins]

Where are the official rules, I see this https://github.com/getumbrel/umbrel-apps?tab=readme-ov-file#a-good-dockerfile

Uses remote assets that are verified against a checksum.

We can do that.

[peterzen]

Where are the official rules, I see this https://github.com/getumbrel/umbrel-apps?tab=readme-ov-file#a-good-dockerfile

Uses remote assets that are verified against a checksum.

Right - nevermind I mixed up Umbrel with the Snap package build, the latter needs the offline build. Apologies for the confusion, it's been a while.

[peterzen]

1. We can't update the docker file without updating the tag if we have to checkout the tag first. This makes things very awkward.

The Dockerfile doesn't need updating for releases, the GH workflow uses the current git ref to tag the docker image:

dcrdex/.github/workflows/release-docker.yml

Line 37 in 552e26d

--tag ${{ env.IMAGE_NAME }}:${{ github.ref_name }} \

To publish the release for Umbrel this needs to be updated:

dcrdex/dcrdex-umbrel/docker-compose.yml

Line 12 in 552e26d

image: decred/dcrdex:v1.0.3@sha256:6c504ae6570ce7753d345cae3c592b411c903430405e1ab9379271124fc44088

If this update goes in as a separate PR after the release then it's less awkward. /dcrdex-umbrel really ought to live in a separate repo, the current situation is not ideal.

Does this make sense?

2. Currently it copies over the user's entire dcrdex folder, which may have sensitive information related to testing, like providers json files that have an api secret. This is also copying the entire local .git history? Probably fine since the instructions are to clone a new repo first.

It's a 2 stage build; stage 1 copies in the current clone, stage 2 starts with a blank image and copies over only the binary from stage 1. So nothing can and up in the final image unless explicitly populated from client/Dockerfile. Sensitive stuff can be added to .dockerignore which docker will then ignore during the COPY in stage 1.

3. When researching almost all suggestions were to get the tag from git if we want to build at the tag.

There are situations like when a docker image is built from several source repos etc, that's when that pattern comes in handy.

[JoeGruffins]

Just updating the docker image we point to. Maybe the current flow is best for umbrel. Not changing for now.

[JoeGruffins]

The Dockerfile doesn't need updating for releases, the GH workflow uses the current git ref to tag the docker image:

We had to do this at one point 85a5783

So sometimes we need to update the Dockerfile right? We were able to get that into the patch but there may be times when we cannot without updating the patch. Also all the versions before we got it in should be broken now.

[peterzen]

So sometimes we need to update the Dockerfile right? We were able to get that into the patch but there may be times when we cannot without updating the patch.

Right, for dependencies etc but a new release on its own doesn't need a Dockerfile update.

Also all the versions before we got it in should be broken now.

You mean the previous Umbrel releases before /dcrdex-umbrel was imported?

[JoeGruffins]

You mean the previous Umbrel releases before /dcrdex-umbrel was imported?

Was there not a release before that change? Anyway merging.