zrhoffman commented on a change in pull request #5938:
URL: https://github.com/apache/trafficcontrol/pull/5938#discussion_r652110854
##########
File path: cache-config/testing/docker/trafficserver/run.sh
##########
@@ -57,31 +83,43 @@ else
source scl_source enable gcc-toolset-9
fi
-initBuildArea
+cd /root
+# prep build environment
+[ -e rpmbuild ] && rm -rf rpmbuild
+[ ! -e rpmbuild ] || { echo "Failed to clean up rpm build directory
'rpmbuild': $?" >&2; exit 1; }
+mkdir -p rpmbuild/{BUILD,BUILDROOT,RPMS,SPECS,SOURCES,SRPMS} || die "Failed to
create build directory '$RPMBUILD': $?"
cd /root/rpmbuild/SOURCES
# clone the trafficserver repo
-git clone https://github.com/apache/trafficserver.git
+git clone https://github.com/apache/trafficserver.git --branch $ATS_VERSION ||
die "Failed to fetch the ATS Source"
+cp /traffic_server_jemalloc .
+
+# patch in the astats plugin
+(cp -far /trafficcontrol/traffic_server/_tsb/src/ats/plugins/astats_over_http \
Review comment:
`-a` implies `-r`, so `cp -far` can be just `cp -fa`.
##########
File path: cache-config/testing/docker/trafficserver/run.sh
##########
@@ -57,31 +83,43 @@ else
source scl_source enable gcc-toolset-9
fi
-initBuildArea
+cd /root
+# prep build environment
+[ -e rpmbuild ] && rm -rf rpmbuild
+[ ! -e rpmbuild ] || { echo "Failed to clean up rpm build directory
'rpmbuild': $?" >&2; exit 1; }
+mkdir -p rpmbuild/{BUILD,BUILDROOT,RPMS,SPECS,SOURCES,SRPMS} || die "Failed to
create build directory '$RPMBUILD': $?"
cd /root/rpmbuild/SOURCES
# clone the trafficserver repo
-git clone https://github.com/apache/trafficserver.git
+git clone https://github.com/apache/trafficserver.git --branch $ATS_VERSION ||
die "Failed to fetch the ATS Source"
+cp /traffic_server_jemalloc .
+
+# patch in the astats plugin
+(cp -far /trafficcontrol/traffic_server/_tsb/src/ats/plugins/astats_over_http \
+ /root/rpmbuild/SOURCES/trafficserver/plugins/astats_over_http
-# build trafficserver version 9
+ed /root/rpmbuild/SOURCES/trafficserver/plugins/Makefile.am << ED
+/stats_over_http/
+a
+include astats_over_http/Makefile.inc
+.
+wQ
+ED
+) || die "Failed to patch in astats_over_http"
+
+# build a trafficserver RPM
rm -f /root/rpmbuild/RPMS/x86_64/trafficserver-*.rpm
cd trafficserver
-git fetch --all
-git checkout $ATS_VERSION
-rpmbuild -bb /trafficserver.spec
+
+rpmbuild -bb ${rpmbuild_openssl} /trafficserver.spec || die "Failed to build
the ATS RPM."
echo "Build completed"
if [[ ! -d /trafficcontrol/dist ]]; then
mkdir /trafficcontrol/dist
fi
-case ${ATS_VERSION:0:1} in
- 8) cp /root/rpmbuild/RPMS/x86_64/trafficserver-8*.rpm /trafficcontrol/dist
- ;;
- 9) cp /root/rpmbuild/RPMS/x86_64/trafficserver-8*.rpm /trafficcontrol/dist
- ;;
- *) echo "Unknown trafficserver version was specified"
- exit 1
- ;;
-esac
+cp /root/rpmbuild/RPMS/x86_64/trafficserver*.rpm /trafficcontrol/dist || \
+ die "Failed to copy the ATS RPM to the dist directory"
Review comment:
`||` implies a second line, so `\` is unnecessary.
##########
File path: cache-config/testing/docker/ort_test/Dockerfile
##########
@@ -28,15 +28,17 @@
# The recommended minimum size for each block devices is 1G.
# For example, `sudo modprobe brd rd_size=1048576 rd_nr=2`
-FROM centos:7
+FROM centos:${RHEL_VERSION:-8}
MAINTAINER [email protected]
+RUN yum -y install epel-release
+
+RUN yum repolist
+
RUN yum install -y \
Review comment:
Since these 3 `RUN` instructions all only perform `yum` commands, can
they be condensed into a single `RUN` instruction for cache busting?
##########
File path: cache-config/testing/docker/trafficserver/run.sh
##########
@@ -57,31 +83,42 @@ else
source scl_source enable gcc-toolset-9
fi
-initBuildArea
+cd /root
+# prep build environment
+[ -e rpmbuild ] && rm -rf rpmbuild
+[ ! -e rpmbuild ] || { echo "Failed to clean up rpm build directory
'rpmbuild': $?" >&2; exit 1; }
+mkdir -p rpmbuild/{BUILD,BUILDROOT,RPMS,SPECS,SOURCES,SRPMS} || die "Failed to
create build directory '$RPMBUILD': $?"
cd /root/rpmbuild/SOURCES
# clone the trafficserver repo
-git clone https://github.com/apache/trafficserver.git
+git clone https://github.com/apache/trafficserver.git --branch $ATS_VERSION ||
die "Failed to fetch the ATS Source"
+cp /traffic_server_jemalloc .
+
+# patch in the astats plugin
+(cp -far /astats_over_http
/root/rpmbuild/SOURCES/trafficserver/plugins/astats_over_http
-# build trafficserver version 9
+ed /root/rpmbuild/SOURCES/trafficserver/plugins/Makefile.am << ED
Review comment:
The stock CentOS 7 and CentOS 8 Docker images do not include `ed`.
##########
File path: cache-config/testing/docker/trafficserver/run.sh
##########
@@ -18,32 +18,58 @@
# under the License.
#
-function initBuildArea() {
- cd /root
-
- # prep build environment
- [ -e rpmbuild ] && rm -rf rpmbuild
- [ ! -e rpmbuild ] || { echo "Failed to clean up rpm build directory
'rpmbuild': $?" >&2; exit 1; }
- mkdir -p rpmbuild/{BUILD,BUILDROOT,RPMS,SPECS,SOURCES,SRPMS} || { echo
"Failed to create build directory '$RPMBUILD': $?" >&2;
- exit 1; }
+function die() {
+ { test -n "$@" && echo "$@"; exit 1; } >&2
}
-setowner() {
- own="$(stat -c%u:%g "$1")"
- shift
- chown -R "${own}" "$@"
-}
-trap 'exit_code=$?; setowner /trafficcontrol /trafficcontrol/dist; exit
$exit_code' EXIT;
-
-case ${ATS_VERSION:0:1} in
- 8) cp /trafficserver-8.spec /trafficserver.spec
- ;;
- 9) cp /trafficserver-9.spec /trafficserver.spec
- ;;
- *) echo "Unknown trafficserver version was specified"
- exit 1
- ;;
-esac
+# sets minimum defaults if these are undefined. These are normally defined in
the
+# environment section of the GHA workflow see,
'.github/workflows/cache-config-tests.yml'
+RHEL_VERSION="${RHEL_VERSION:-8}"
+ATS_VERSION="${ATS_VERSION:-8.1.x}"
+
+echo "RHEL_VERSION:${RHEL_VERSION}"
+echo "ATS_VERSION:${ATS_VERSION}"
+
+mkdir -p /opt/build
+cd /opt/build
+
+# build openssl 1.1.1 if RHEL_VERSION is not 8 or greater.
+if [[ ${RHEL_VERSION%%.*} -le 7 ]]; then
+ git clone $OPENSSL_URL --branch $OPENSSL_TAG || die "Failed to fetch the
OpenSSL Source."
+ (
+ cd /opt/build/openssl && \
+ ./config --prefix=/opt/trafficserver/openssl
--openssldir=/opt/trafficserver zlib && \
+ make -j$(nproc) && \
Review comment:
No need to include `\` after `&&` outside of Dockerfiles.
##########
File path: cache-config/testing/docker/ort_test/Dockerfile
##########
@@ -28,15 +28,17 @@
# The recommended minimum size for each block devices is 1G.
# For example, `sudo modprobe brd rd_size=1048576 rd_nr=2`
-FROM centos:7
+FROM centos:${RHEL_VERSION:-8}
MAINTAINER [email protected]
+RUN yum -y install epel-release
+
+RUN yum repolist
+
RUN yum install -y \
-
https://download.postgresql.org/pub/repos/yum/reporpms/EL-7-x86_64/pgdg-redhat-repo-latest.noarch.rpm
\
- epel-release initscripts postgresql13.x86_64 git gcc lua-5.1.4-15.el7
lua-devel-5.1.4-15.el7 \
- ImageMagick-c++-devel && \
- # jq is used in run.sh to update tc-fixtures.json with the ATS RPM version
- yum install -y jq
+ initscripts git jq \
+ git gcc jq \
Review comment:
`jq` is listed twice
##########
File path: cache-config/testing/docker/docker-compose-ats-build.yml
##########
@@ -27,11 +27,14 @@ services:
trafficserver_build:
environment:
- - 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
build:
context: ../../..
dockerfile: cache-config/testing/docker/trafficserver/Dockerfile
- args:
- RHEL_VERSION: ${RHEL_VERSION:-7}
Review comment:
Why remove the `RHEL_VERSION` build arg? Without it, `RHEL_VERSION` is
not inherited from the environment, which is how the CDN in a Box Makefile
passes the `RHEL_VERSION` build arg to Dockerfiles.
##########
File path: cache-config/testing/docker/ort_test/Dockerfile
##########
@@ -28,15 +28,17 @@
# The recommended minimum size for each block devices is 1G.
# For example, `sudo modprobe brd rd_size=1048576 rd_nr=2`
-FROM centos:7
+FROM centos:${RHEL_VERSION:-8}
Review comment:
Because there is no
[`ARG`](https://docs.docker.com/engine/reference/builder/#arg) instruction for
`RHEL_VERSION`, there is no way to set `RHEL_VERSION` to `7` at build time.
##########
File path: cache-config/testing/docker/trafficserver/Dockerfile
##########
@@ -17,54 +17,106 @@
###############################################################
# Dockerfile to build Traffic Server RPM
-# Based on CentOS 7 for ATS RPM ot match the ort_test
-# container running CentOS 7.
+# Defaults to CentOS 8, see RHEL_VERSION
###############################################################
-ARG RHEL_VERSION=7
+# This sets the minimum RHEL_VERSION to 8
+# You may override and use RHEL_VERSION 7 using:
+# 'docker-compose build --build-args RHEL_VERSION=7 ats_build'
+# and followed by
+# 'docker-compose run -e RHEL_VERSION=7 ats_build'
+#
+# For the GHA workflow, override this in
.github/workflows/cache-config-tests.yml
+ARG RHEL_VERSION=${RHEL_VERSION:-8}
Review comment:
The `$RHEL_VERSION` build arg is always undefined before this line.
Because `${RHEL_VERSION:-8}` always defaults to `8`, it can be just
`RHEL_VERSION=8`.
##########
File path: .github/workflows/ciab.yaml
##########
@@ -45,6 +45,7 @@ on:
- 'NOTICE'
- 'traffic_control/java/**'
- 'cache-config/testing/**'
+ 'cache-config/testing/docker/trafficserver/**'
Review comment:
This line should be prefixed with `-` to make it a separate YAML array
element.
##########
File path: cache-config/testing/docker/ort_test/Dockerfile
##########
@@ -28,15 +28,17 @@
# The recommended minimum size for each block devices is 1G.
# For example, `sudo modprobe brd rd_size=1048576 rd_nr=2`
-FROM centos:7
+FROM centos:${RHEL_VERSION:-8}
MAINTAINER [email protected]
+RUN yum -y install epel-release
+
+RUN yum repolist
+
RUN yum install -y \
-
https://download.postgresql.org/pub/repos/yum/reporpms/EL-7-x86_64/pgdg-redhat-repo-latest.noarch.rpm
\
- epel-release initscripts postgresql13.x86_64 git gcc lua-5.1.4-15.el7
lua-devel-5.1.4-15.el7 \
- ImageMagick-c++-devel && \
- # jq is used in run.sh to update tc-fixtures.json with the ATS RPM version
- yum install -y jq
+ initscripts git jq \
+ git gcc jq \
Review comment:
`git` is listed twice
##########
File path: cache-config/testing/docker/trafficserver/Dockerfile
##########
@@ -17,54 +17,106 @@
###############################################################
# Dockerfile to build Traffic Server RPM
-# Based on CentOS 7 for ATS RPM ot match the ort_test
-# container running CentOS 7.
+# Defaults to CentOS 8, see RHEL_VERSION
###############################################################
-ARG RHEL_VERSION=7
+# This sets the minimum RHEL_VERSION to 8
+# You may override and use RHEL_VERSION 7 using:
+# 'docker-compose build --build-args RHEL_VERSION=7 ats_build'
+# and followed by
+# 'docker-compose run -e RHEL_VERSION=7 ats_build'
+#
+# For the GHA workflow, override this in
.github/workflows/cache-config-tests.yml
+ARG RHEL_VERSION=${RHEL_VERSION:-8}
FROM centos:${RHEL_VERSION}
-ARG RHEL_VERSION=7
+ARG RHEL_VERSION=${RHEL_VERSION:-8}
Review comment:
Same here. The `$RHEL_VERSION` build arg is always undefined before this
line. Because `${RHEL_VERSION:-8}` always defaults to `8`, it can be just
`RHEL_VERSION=8`.
##########
File path: .github/workflows/ciab.yaml
##########
@@ -69,6 +70,7 @@ on:
- 'NOTICE'
- 'traffic_control/java/**'
- 'cache-config/testing/**'
+ - 'cache-config/testing/docker/trafficserver**'
Review comment:
Since this section is `on.pull_request.paths-ignore`,
`cache-config/testing/docker/trafficserver/**` should be prefixed with `!` to
register the exception.
##########
File path: cache-config/testing/docker/trafficserver/.dependency_license
##########
@@ -0,0 +1,23 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+# SPDX-License-Identifier: Apache-2.0
+
+# Patch files, cannot bear comments
+cjose.pic.patch, Apache-2.0
Review comment:
CJOSE is MIT-licensed.
##########
File path: .github/workflows/ciab.yaml
##########
@@ -45,6 +45,7 @@ on:
- 'NOTICE'
- 'traffic_control/java/**'
- 'cache-config/testing/**'
+ 'cache-config/testing/docker/trafficserver/**'
Review comment:
Since this section is `on.push.paths-ignore`,
`cache-config/testing/docker/trafficserver/**` should be prefixed with `!` to
register the exception.
--
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]
