zrhoffman commented on a change in pull request #5938:
URL: https://github.com/apache/trafficcontrol/pull/5938#discussion_r653847725
##########
File path: cache-config/testing/docker/docker-compose-ats-build.yml
##########
@@ -27,11 +27,19 @@ services:
trafficserver_build:
environment:
+ - RHEL_VERSION=8
- ATS_VERSION=8.1.x
+ - CJOSE_URL=https://github.com/cisco/cjose
+ - CJOSE_TAG=latest
+ - JANSSON_URL=https://github.com/akheron/jansson
+ - JANSSON_TAG=v2.11
+ - OPENSSL_URL=https://github.com/openssl/openssl
+ - OPENSSL_TAG=OpenSSL_1_1_1
+ - RUN_ATS_UNIT_TESTS=true
Review comment:
`RUN_ATS_UNIT_TESTS` should be false when run from GitHub Actions.
##########
File path: cache-config/testing/docker/ort_test/Dockerfile
##########
@@ -28,15 +23,16 @@
# The recommended minimum size for each block devices is 1G.
# For example, `sudo modprobe brd rd_size=1048576 rd_nr=2`
-FROM centos:7
+ARG RHEL_VERSION=${RHEL_VERSION}
+FROM centos:${RHEL_VERSION}
+ARG RHEL_VERSION=${RHEL_VERSION}
Review comment:
Build args are cleared at the beginning of each Docker layer, so
```dockerfile
ARG RHEL_VERSION=${RHEL_VERSION}
```
expands to
```dockerfile
ARG RHEL_VERSION=
```
which is functionally equivalent to
```dockerfile
ARG RHEL_VERSION
```
Trying to build the Dockerfile without providing a build arg fails:
```shell
[user@computer trafficcontrol]$ cd cache-config/testing/docker/ort_test
[user@computer ort_test]$ docker build .
[+] Building 0.8s (2/2) FINISHED
=> [internal] load build definition from Dockerfile
0.4s
=> => transferring dockerfile: 38B
0.0s
=> [internal] load .dockerignore
0.6s
=> => transferring context: 2B
0.0s
failed to solve with frontend dockerfile.v0: failed to create LLB
definition: failed to parse stage name "centos:": invalid reference format
```
##########
File path: cache-config/testing/docker/docker-compose-ats-build.yml
##########
@@ -27,11 +27,19 @@ services:
trafficserver_build:
environment:
+ - RHEL_VERSION=8
Review comment:
Building the image using build arg `RHEL_VERSION=7` and running using
that image and environment variable `RHEL_VERSION=8` (and vice versa) causes
the ATS RPM to fail to build. So, either
* `RHEL_VERSION` should be copied from the build arg to the environment at
Docker image build time like the t3c tests `traffic_ops` Dockerfile does:
https://github.com/apache/trafficcontrol/blob/251db037b425793229cc2239596e0b183b288a64/cache-config/testing/docker/traffic_ops/Dockerfile#L27
or
* `RHEL_VERSION` can be detected from the OS at run time:
https://github.com/apache/trafficcontrol/blob/251db037b425793229cc2239596e0b183b288a64/build/functions.sh#L136-L137
##########
File path: cache-config/testing/docker/docker-compose-ats-build.yml
##########
@@ -27,11 +27,19 @@ services:
trafficserver_build:
environment:
+ - RHEL_VERSION=8
Review comment:
`RHEL_VERSION` is still defined in the environment in the compose file,
which overrides `RHEL_VERSION` as set by an `ENV` instruction in the
Dockerfile, so it should still be removed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
