docker_run.sh: Make docker image root name configurable

[AlessandroBono]

Otherwise the script won't find the docker image built with
docker_build.sh. The docker images list the repository as
follows:

$ docker images
REPOSITORY                     TAG    IMAGE ID     CREATED      SIZE
ghcr.io/oss-review-toolkit/ort latest d0c66bb06c31 24 hours ago 3.06GB

[sschuberth]

Originally, this script was supposed to work with locally built images. I'm not sure we should hard-code the registry now here. I'd prefer to manually pull before instead.

[AlessandroBono]

I might be missing something. I locally built an image with docker_build.sh and only ghcr.io/oss-review-toolkit/ort and similar names are listed. I thought that the two scripts were bound together. How should I locally build images?

[sschuberth]

It's correct that these two script should work together. I believe DOCKER_IMAGE_ROOT shouldn't have been introduced in docker_build.sh to begin with. Why was it required, @heliocastro?

[heliocastro]

Because reuse of the docker build script inside organizations.
For example, there are specific restrictions on my company to access the external internet, so we have an internal registry where we publish the internally built images.
And since it is a different organization, the root is different.

[heliocastro]

Simply add this at the beginning of the script:

DOCKER_IMAGE_ROOT=${DOCKER_IMAGE_ROOT:-ghcr.io/oss-review-toolkit}

And then replace the last line for

${DOCKER_IMAGE_ROOT}/ort

[sschuberth]

But why default to "ghcr.io/oss-review-toolkit" if DOCKER_IMAGE_ROOT is not set? That makes local building and running really inconvenient.

[AlessandroBono]

Any news on this?

I think the discussion about the default prefix should not concern this patch. The prefix is already defined in docker_build.sh and it is already in main. This PR is just about make docker_run.sh work again with the companion docker_build.sh script.

The default prefix can be changed in a subsequent PR in both scripts.

[sschuberth]

I think the discussion about the default prefix should not concern this patch.

I tend to disagree, as you're introducing code that would not be necessary if instead the default prefix would be removed from scripts/docker_build.sh, which IMO is the more correct thing to do.

[heliocastro]

Do you mean remove DOCKER_IMAGE_ROOT ? That's would be a bad idea for several reasons.
I placed it there to really be useful in such cases:

1 - Every documentation that we will write from now on, will point out to a default, that's today is ghcr now. So everyone that will try any example will always try first ghcr.io/oss-review-toolkit/ort first.
If you build locally, and then try to run, the very first thing that will happens will be pull again from the internet. Worst, as we not publish Arm images, the very first time it pull wrongly from internet will bring a amd64 image that will not run well. This just because we made a silly mistake of use prefixed than without one.
And write documentation for "if we built locally do that, if you get from internet, do this" will not help, as we barely can write documentation.

2 - As primary ORT users are companies that usually don't trust anywhere else than their own buildsystems, they will build the image locally. And they will not use the default registry, but one internal probably. Means, for this usage, removing the prefix alteration will just make our script mostly useless and someone will need fork and create his own script to build.
As a direct example, i use the build_script for build local images using our buildsystem and arm64 images on a mac machine, passing out local registry, and copy for all necessary locations the generated images, properly tagged.

Yes, there's possibility to tag image later, and any other possible tricks, but remove the prefix will not benefit in anything, is just add another confusion point.

[sschuberth]

I'm not proposing to remove the DOCKER_IMAGE_ROOT variable per se, but to change its default its default value from "ghcr.io/oss-review-toolkit" to the empty string. Because like you say

[users] will build the image locally. And they will [use a registry that is] internal probably

My goal would be that running docker_build.sh / docker_run.sh work locally by default, and if DOCKER_IMAGE_ROOT is set, a remote registry is used.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3b353f5) 67.80% compared to head (c8a5888) 67.43%.

❗ Current head c8a5888 differs from pull request most recent head 4ee60d3. Consider uploading reports for the commit 4ee60d3 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7729      +/-   ##
============================================
- Coverage     67.80%   67.43%   -0.38%     
- Complexity     2045     2084      +39     
============================================
  Files           352      352              
  Lines         16828    17112     +284     
  Branches       2380     2476      +96     
============================================
+ Hits          11411    11540     +129     
- Misses         4427     4567     +140     
- Partials        990     1005      +15     

Flag	Coverage Δ
funTest-docker	67.85% <ø> (ø)
funTest-non-docker	36.44% <ø> (ø)
test	34.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[heliocastro]

Looks good to me.
I'll recommend pass shellcheck over the script just to do some sanitising,