[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4972: CDN-in-a-Box readiness CI test

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

ocket8888 commented on a change in pull request #4972:
URL: https://github.com/apache/trafficcontrol/pull/4972#discussion_r473261888



##########
File path: .github/actions/build-rpms/main.js
##########
@@ -0,0 +1,37 @@
+/*
+* Licensed 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.
+*/
+
+const child_process = require("child_process");
+const spawnArgs = {
+	stdio: "inherit",
+	stderr: "inherit",
+};
+
+let atcComponent = process.env.ATC_COMPONENT;
+const dockerComposeArgs = ["-f", `${process.env.GITHUB_WORKSPACE}/infrastructure/docker/build/docker-compose.yml`, "run", "--rm"];
+if (typeof atcComponent !== "string" || atcComponent.length === 0) {
+	console.error("Missing environment variable ATC_COMPONENT");
+	process.exit(1);
+}
+const nonRpmComponents = ["source", "weasel", "docs"];
+if (nonRpmComponents.indexOf(atcComponent) === -1) {
+	atcComponent += "_build";
+}
+dockerComposeArgs.push(atcComponent);
+const proc = child_process.spawnSync(
+	"docker-compose",
+	dockerComposeArgs,
+	spawnArgs
+);
+process.exit(proc.status);

Review comment:
       Text files must include a terminating newline.

##########
File path: .github/actions/build-rpms/main.js
##########
@@ -0,0 +1,37 @@
+/*
+* Licensed 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.
+*/
+

Review comment:
       `use "strict";`?

##########
File path: .github/workflows/ciab.yaml
##########
@@ -0,0 +1,194 @@
+# 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: CDN-in-a-Box CI
+
+on:
+  push:
+  create:
+  pull_request:
+    types: [opened, reopened, edited, synchronize]
+
+jobs:
+  source:
+    runs-on: ubuntu-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@master
+    - 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/apache-trafficcontrol-*.tar.gz
+
+  traffic_monitor:
+    runs-on: ubuntu-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@master
+    - 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 }}-*.rpm

Review comment:
       if you aren't worried about hosting source RPMs as artifacts, it might be easier to archive `infrastructure/cdn-in-a-box/${{ github.job }}/${{ github.job }}.rpm`

##########
File path: .github/actions/run-ciab/entrypoint.sh
##########
@@ -0,0 +1,52 @@
+#!/bin/sh -l
+# 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.
+
+set -ex;
+export COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 # use Docker BuildKit for better image building performance
+(
+cd dist;
+mv -- */*.rpm .;
+);
+
+docker-compose --version;
+cd infrastructure/cdn-in-a-box;
+make; # All RPMs should have already been built
+
+docker images;
+logged_services='trafficrouter readiness';
+other_services='dns edge enroller mid origin trafficmonitor trafficops trafficops-perl trafficstats trafficvault';
+docker_compose='docker-compose -f ./docker-compose.yml -f ./docker-compose.readiness.yml';
+time $docker_compose build --parallel $logged_services $other_services;
+$docker_compose up -d $logged_services $other_services;
+$docker_compose logs -f $logged_services &

Review comment:
       It actually also looks like you explicitly call `docker-compose logs` later for other services. Instead of waiting on `docker-compose ps -q readiness`, you could set a timeout on `docker-compose logs -f readiness` to get it to print out the logs without needing job control or mixing output.
   
   also, why are you logging traffic router here? I would guess it's for context to see if something's going wrong in Traffic Router, but if you output the logs later anyway, then you'll still get that information, so instead you could only report about the thing you're actually waiting for and then dump all the logs - since it's not more likely for the problem to occur in TR than any other necessary component (and most are).

##########
File path: .github/actions/run-ciab/entrypoint.sh
##########
@@ -0,0 +1,52 @@
+#!/bin/sh -l
+# 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.
+
+set -ex;
+export COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 # use Docker BuildKit for better image building performance
+(
+cd dist;
+mv -- */*.rpm .;

Review comment:
       Does our build system output rpms in subdirectories of `dist`? I don't believe I've ever seen that.

##########
File path: .github/actions/run-ciab/entrypoint.sh
##########
@@ -0,0 +1,52 @@
+#!/bin/sh -l
+# 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.
+
+set -ex;
+export COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 # use Docker BuildKit for better image building performance
+(
+cd dist;
+mv -- */*.rpm .;
+);
+
+docker-compose --version;
+cd infrastructure/cdn-in-a-box;
+make; # All RPMs should have already been built
+
+docker images;
+logged_services='trafficrouter readiness';
+other_services='dns edge enroller mid origin trafficmonitor trafficops trafficops-perl trafficstats trafficvault';
+docker_compose='docker-compose -f ./docker-compose.yml -f ./docker-compose.readiness.yml';
+time $docker_compose build --parallel $logged_services $other_services;
+$docker_compose up -d $logged_services $other_services;
+$docker_compose logs -f $logged_services &
+
+set +e;
+container_exit_code="$(timeout 10m docker wait "$($docker_compose ps -q readiness)")"

Review comment:
       ten minutes sounds like plenty, but I'd like to know if there was any specific metric you used to determine that number.

##########
File path: .github/actions/build-rpms/main.js
##########
@@ -0,0 +1,37 @@
+/*
+* Licensed 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.
+*/
+
+const child_process = require("child_process");
+const spawnArgs = {
+	stdio: "inherit",
+	stderr: "inherit",
+};
+
+let atcComponent = process.env.ATC_COMPONENT;
+const dockerComposeArgs = ["-f", `${process.env.GITHUB_WORKSPACE}/infrastructure/docker/build/docker-compose.yml`, "run", "--rm"];
+if (typeof atcComponent !== "string" || atcComponent.length === 0) {
+	console.error("Missing environment variable ATC_COMPONENT");
+	process.exit(1);
+}
+const nonRpmComponents = ["source", "weasel", "docs"];

Review comment:
       I don't think there's a need to support these.
   
   GHA will always run with a specific git ref, so if you want the source it should be as easy as `git checkout {{ref}}`. 
   
   Weasel is already covered by our weasel action - its output could be added as an artifact, if desired, but otherwise the text log is already there, even in raw form for direct linking.
   
   The docs would be better handled by [a dedicated docs workflow](https://github.com/ocket8888/trafficcontrol/blob/master/.github/workflows/docs.yml) which not only checks if they build, but also highlights errors and warnings inline for developers. To output docs artifacts in arbitrary format, see [the `sphin-action` action's README section on the topic of pairing with an artifact uploading action](https://github.com/ammaraskar/sphinx-action#great-actions-to-pair-with)

##########
File path: .github/actions/run-ciab/entrypoint.sh
##########
@@ -0,0 +1,52 @@
+#!/bin/sh -l
+# 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.
+
+set -ex;
+export COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 # use Docker BuildKit for better image building performance
+(
+cd dist;
+mv -- */*.rpm .;
+);
+
+docker-compose --version;
+cd infrastructure/cdn-in-a-box;
+make; # All RPMs should have already been built
+
+docker images;
+logged_services='trafficrouter readiness';
+other_services='dns edge enroller mid origin trafficmonitor trafficops trafficops-perl trafficstats trafficvault';
+docker_compose='docker-compose -f ./docker-compose.yml -f ./docker-compose.readiness.yml';
+time $docker_compose build --parallel $logged_services $other_services;
+$docker_compose up -d $logged_services $other_services;
+$docker_compose logs -f $logged_services &

Review comment:
       If I were you I'd consider redirecting the logs output to files and reading them at a later time. Just because otherwise it can get hard to find the script execution logs in the unstoppable tide of docker-compose logs. Having things clearly sectioned makes things easier.

##########
File path: pkg
##########
@@ -15,7 +15,7 @@
 # Files are relative to this script directory.
 SELF="${BASH_SOURCE[0]}"
 cd "$( dirname "${BASH_SOURCE[0]}" )"
-COMPOSE_FILE=./infrastructure/docker/build/docker-compose.yml
+COMPOSE_FILE=infrastructure/docker/build/docker-compose.yml

Review comment:
       Was this not working for some reason?

##########
File path: infrastructure/cdn-in-a-box/edge/Dockerfile
##########
@@ -0,0 +1,81 @@
+# Licensed to the Apache Software Foundation (ASF) under one

Review comment:
       Why split these up?

##########
File path: .github/actions/run-ciab/entrypoint.sh
##########
@@ -0,0 +1,52 @@
+#!/bin/sh -l
+# 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.
+
+set -ex;
+export COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 # use Docker BuildKit for better image building performance
+(
+cd dist;
+mv -- */*.rpm .;
+);
+
+docker-compose --version;
+cd infrastructure/cdn-in-a-box;
+make; # All RPMs should have already been built
+
+docker images;
+logged_services='trafficrouter readiness';
+other_services='dns edge enroller mid origin trafficmonitor trafficops trafficops-perl trafficstats trafficvault';
+docker_compose='docker-compose -f ./docker-compose.yml -f ./docker-compose.readiness.yml';
+time $docker_compose build --parallel $logged_services $other_services;
+$docker_compose up -d $logged_services $other_services;
+$docker_compose logs -f $logged_services &
+
+set +e;
+container_exit_code="$(timeout 10m docker wait "$($docker_compose ps -q readiness)")"
+exit_code=$?;
+set -e;
+if [ $exit_code -ne 0 ]; then
+	echo "CDN-in-a-Box didn't become ready within 10 minutes - exiting" >&2;
+  $docker_compose --no-ansi logs --no-color $other_services;
+elif [ "$container_exit_code" -ne 0 ]; then
+	echo 'Readiness container exited with an error' >&2;
+  $docker_compose --no-ansi logs --no-color $other_services;
+	exit_code="$container_exit_code";

Review comment:
       You have mixed tab-based and space-based indentation in here. Tabs >>>>>>>>>>>> spaces, but more importantly it'd be nice to be consistent, whichever you pick.




----------------------------------------------------------------
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