[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5706: Add an ORT integration tests GHA workflow

[GitBox <gi...@apache.org>]

zrhoffman commented on a change in pull request #5706:
URL: https://github.com/apache/trafficcontrol/pull/5706#discussion_r607598107



##########
File path: traffic_ops_ort/testing/docker/trafficserver/run.sh
##########
@@ -0,0 +1,78 @@
+#!/bin/bash
+#
+# 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.
+#
+
+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; }
+}
+
+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
+
+echo "Building a RPM for ATS version: $ATS_VERSION"
+
+# add the 'ats' user
+id ats &>/dev/null || /usr/sbin/useradd -u 176 -r ats -s /sbin/nologin -d /
+
+# setup the environment to use the devtoolset-9 tools.
+source scl_source enable devtoolset-9 
+
+initBuildArea
+
+cd /root/rpmbuild/SOURCES
+# clone the trafficserver repo
+git clone https://github.com/apache/trafficserver.git
+
+# build trafficserver version 9
+rm -f /root/rpmbuild/RPMS/x86_64/trafficserver-*.rpm
+cd trafficserver
+git fetch --all
+git checkout $ATS_VERSION
+rpmbuild -bb /trafficserver.spec
+
+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

Review comment:
       > `trafficserver-8*.rpm`
   
   Should this be `trafficserver-9`?

##########
File path: traffic_ops_ort/testing/docker/trafficserver/trafficserver-8.spec
##########
@@ -0,0 +1,145 @@
+# 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
+#
+%global src %{_topdir}/SOURCES/trafficserver
+%global git_args --git-dir="%{src}/.git" --work-tree="%{src}"
+%global git_tag %(git %{git_args} describe --long |      sed 's/^\\\(.*\\\)-\\\([0-9]\\\+\\\)-g\\\([0-9a-f]\\\+\\\)$/\\\1/' | sed 's/-/_/')
+%global distance %(git %{git_args} describe --long | sed 's/^\\\(.*\\\)-\\\([0-9]\\\+\\\)-g\\\([0-9a-f]\\\+\\\)$/\\\2/')
+%global commit %(git %{git_args} describe --long |   sed 's/^\\\(.*\\\)-\\\([0-9]\\\+\\\)-g\\\([0-9a-f]\\\+\\\)$/\\\3/')

Review comment:
       These regexes are a little hard to read with all of the backslashes. If these lines used `sed -E` instead of `sed`, most of the backslashes could be removed.

##########
File path: .github/workflows/traffic-ops-ort-tests.yml
##########
@@ -0,0 +1,140 @@
+# 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.
+
+name: Traffic Ops ORT integration tests
+
+on: 
+  push:
+  workflow_dispatch:
+    paths:
+      - .github/actions/todb-init/**
+      - .github/workflows/traffic-ops-ort.yml

Review comment:
       The `traffic-ops-ort-tests.yml` workflow should also be triggered when files in the `to-ort-integration-tests` action are modified.

##########
File path: .github/workflows/traffic-ops-ort-tests.yml
##########
@@ -0,0 +1,140 @@
+# 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.
+
+name: Traffic Ops ORT integration tests
+
+on: 
+  push:
+  workflow_dispatch:
+    paths:
+      - .github/actions/todb-init/**
+      - .github/workflows/traffic-ops-ort.yml

Review comment:
       Path should be `.github/workflows/traffic-ops-ort-tests.yml`

##########
File path: traffic_ops_ort/testing/docker/trafficserver/trafficserver-9.spec
##########
@@ -0,0 +1,147 @@
+#
+# 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
+
+%global src %{_topdir}/SOURCES/trafficserver
+%global git_args --git-dir="%{src}/.git" --work-tree="%{src}"
+%global git_tag %(git %{git_args} describe --long |      sed 's/^\\\(.*\\\)-\\\([0-9]\\\+\\\)-g\\\([0-9a-f]\\\+\\\)$/\\\1/' | sed 's/-/_/')
+%global distance %(git %{git_args} describe --long | sed 's/^\\\(.*\\\)-\\\([0-9]\\\+\\\)-g\\\([0-9a-f]\\\+\\\)$/\\\2/')
+%global commit %(git %{git_args} describe --long |   sed 's/^\\\(.*\\\)-\\\([0-9]\\\+\\\)-g\\\([0-9a-f]\\\+\\\)$/\\\3/')

Review comment:
       Same as above, consider using `sed -E` to reduce backslashes and increase readability.

##########
File path: .github/actions/build-ats-test-rpm/action.yaml
##########
@@ -0,0 +1,22 @@
+# 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.
+
+name: 'build-ats-test-rpm'
+description: 'Builds an ATS RPM used for ORT Integration tests.'
+runs:
+  using: 'node12'
+  main: 'main.js'

Review comment:
       In addition to Docker actions and JavaScript actions, GHA now supports [composite run steps](https://docs.github.com/en/actions/creating-actions/creating-a-composite-run-steps-action#creating-an-action-metadata-file) actions (which were [not around](https://github.blog/changelog/2020-08-07-github-actions-composite-run-steps/) when we started migrating to GitHub Actions). So the JavaScript action that you have already is fine as far as I'm concerned, but if you want to do a composite run steps action, that option is out there, too.

##########
File path: .github/workflows/traffic-ops-ort-tests.yml
##########
@@ -0,0 +1,140 @@
+# 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.
+
+name: Traffic Ops ORT integration tests
+
+on: 
+  push:
+  workflow_dispatch:
+    paths:
+      - .github/actions/todb-init/**
+      - .github/workflows/traffic-ops-ort.yml
+      - go.mod
+      - go.sum
+      - GO_VERSION
+      - traffic_ops/*client/**.go
+      - traffic_ops/testing/api/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/**.go
+      - vendor/**.go
+      - vendor/modules.txt
+  create:
+  pull_request:
+    paths:
+      - .github/actions/todb-init/**
+      - .github/workflows/traffic-ops-ort.yml
+      - go.mod
+      - go.sum
+      - GO_VERSION
+      - traffic_ops/*client/**.go
+      - traffic_ops/testing/api/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/**.go
+      - vendor/**.go
+      - vendor/modules.txt
+    types: [opened, reopened, ready_for_review, synchronize]
+
+jobs:
+
+  traffic_ops:
+    if: github.event.pull_request.draft == false
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+      - name: Build RPM
+        uses: ./.github/actions/build-rpms
+        env:
+          ATC_COMPONENT: ${{ github.job }}
+      - name: Upload RPM
+        uses: actions/upload-artifact@v2
+        with:
+          name: ${{ github.job }}
+          path: ${{ github.workspace }}/dist/${{ github.job }}-*.x86_64.rpm
+
+  traffic_ops_ort:
+    if: github.event.pull_request.draft == false
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+      - name: Build RPM
+        uses: ./.github/actions/build-rpms
+        env:
+          ATC_COMPONENT: ${{ github.job }}
+      - name: Upload RPM
+        uses: actions/upload-artifact@v2
+        with:
+          name: ${{ github.job }}
+          path: ${{ github.workspace }}/dist/${{ github.job }}-*.x86_64.rpm
+    
+  trafficserver:
+    if: github.event.pull_request.draft == false
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+      - name: Build ATS RPM
+        uses: ./.github/actions/build-ats-test-rpm
+        env:
+          ATC_COMPONENT: ${{ github.job }}

Review comment:
       The `trafficserver` RPM takes 11m20s to build, about 10 minutes longer than the `traffic_ops` or `traffic_ops_ort` RPMs do. Can we cache the `trafficserver` RPM and only build on a cache miss?
   
   The cache key should include the hash of the commit the RPM was built from. With some text manipulation, the latest 8.1.x or 9.1.x commit can be found from https://github.com/apache/trafficserver/info/refs?service=git-upload-pack.
   
   A shell command can get that commit hash, then that step can make the result available [as an output](https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#using-workflow-commands-to-access-toolkit-functions) for use in a later step. Here is the `go.vet.yml` workflow setting and using an output:
   https://github.com/apache/trafficcontrol/blob/b31db78965ee66dd5abea1508f732bdc42cafe0c/.github/workflows/go.vet.yml#L43-L47

##########
File path: .github/workflows/traffic-ops-ort-tests.yml
##########
@@ -0,0 +1,140 @@
+# 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.
+
+name: Traffic Ops ORT integration tests
+
+on: 
+  push:
+  workflow_dispatch:
+    paths:
+      - .github/actions/todb-init/**
+      - .github/workflows/traffic-ops-ort.yml
+      - go.mod
+      - go.sum
+      - GO_VERSION
+      - traffic_ops/*client/**.go
+      - traffic_ops/testing/api/**.go
+      - traffic_ops/traffic_ops_golang/**.go
+      - traffic_ops_ort/**.go
+      - vendor/**.go
+      - vendor/modules.txt
+  create:
+  pull_request:
+    paths:
+      - .github/actions/todb-init/**
+      - .github/workflows/traffic-ops-ort.yml

Review comment:
       The path should be `.github/workflows/traffic-ops-ort-tests.yml` here, too




-- 
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:
users@infra.apache.org