Clean up version-number handling in the Go code

 1. The ldflags thing is a sham, we must always get it from the
    ambassador.version file.
     + And therefore we should not set -ldflags in Dockerfile or
       builder.sh.  And because the 'version' build-arg is now unused,
       remove that too.
 2. Use go-shellquote instead of our own janky de-quoting.
 3. Simplify by using os.ReadFile.
 4. Don't use global variables.
 5. Emphasize that the version-parsing code must be kept in-sync with
    VERSION.py

Signed-off-by: Luke Shumaker <[email protected]>

builder/Dockerfile

@@ -66,8 +66,6 @@ ENTRYPOINT [ "/bin/bash" ]
 
 FROM ${builderbase} as golang
 
-ARG version="i-forgot-to-set-build-arg-version"
-
 WORKDIR /go
 
 ENV PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/buildroot/bin
@@ -80,7 +78,7 @@ ADD vendor vendor
 ADD go.mod go.mod
 ADD go.sum go.sum
 RUN mkdir -p /go/bin && \
-	time go build -ldflags="-X 'main.Version=${version}'" -mod=vendor -o /go/bin/ ./cmd/...
+	time go build -mod=vendor -o /go/bin/ ./cmd/...
 
 ########################################
 # The artifact build stage

builder/builder.mk

@@ -280,7 +280,6 @@ docker/$(LCNAME).docker.stamp: %/$(LCNAME).docker.stamp: %/base-envoy.docker.tag
 	  docker build -f ${BUILDER_HOME}/Dockerfile . \
 	    --build-arg=envoy="$$(cat $*/base-envoy.docker)" \
 	    --build-arg=builderbase="$$(cat $*/builder-base.docker)" \
-	    --build-arg=version="$(BUILD_VERSION)" \
 	    --build-arg=py_version="$$(cat build-aux/py-version.txt)" \
 	    --target=ambassador \
 	    --iidfile=$@; }

builder/builder.sh

@@ -462,15 +462,14 @@ case "${cmd}" in
 
         for MODDIR in $(find-modules); do
             module=$(basename ${MODDIR})
-            eval "$(grep BUILD_VERSION apro.version 2>/dev/null)" # this will `eval ''` for OSS-only builds, leaving BUILD_VERSION unset; dont embed the version-number in OSS Go binaries
 
             if [ -e ${module}.dirty ] || ([ "$module" != ambassador ] && [ -e go.dirty ]) ; then
                 if [ -e "${MODDIR}/go.mod" ]; then
                     printf "${CYN}==> ${GRN}Building ${BLU}${module}${GRN} go code${END}\n"
                     echo_on
                     mkdir -p /buildroot/bin
                     TIMEFORMAT="     (go build took %1R seconds)"
-                    (cd ${MODDIR} && time go build -trimpath ${BUILD_VERSION:+ -ldflags "-X main.Version=$BUILD_VERSION" } -o /buildroot/bin ./cmd/...) || exit 1
+                    (cd ${MODDIR} && time go build -trimpath -o /buildroot/bin ./cmd/...) || exit 1
                     TIMEFORMAT="     (${MODDIR}/post-compile took %1R seconds)"
                     if [ -e ${MODDIR}/post-compile.sh ]; then (cd ${MODDIR} && time bash post-compile.sh); fi
                     unset TIMEFORMAT

cmd/busyambassador/main.go

@@ -6,110 +6,62 @@
 package main
 
 import (
-	"bufio"
+	// stdlib
 	"context"
 	"fmt"
 	"os"
 	"strings"
 
+	// 3rd-party libs
+	"github.com/kballard/go-shellquote"
+
+	// 1st-party libs
 	"github.com/datawire/ambassador/v2/pkg/busy"
 	"github.com/datawire/ambassador/v2/pkg/environment"
 
-	amb_agent "github.com/datawire/ambassador/v2/cmd/agent"
+	// commands
+	"github.com/datawire/ambassador/v2/cmd/agent"
 	"github.com/datawire/ambassador/v2/cmd/ambex"
 	"github.com/datawire/ambassador/v2/cmd/apiext"
 	"github.com/datawire/ambassador/v2/cmd/entrypoint"
 	"github.com/datawire/ambassador/v2/cmd/kubestatus"
 	"github.com/datawire/ambassador/v2/cmd/reproducer"
 )
 
-// Version is inserted at build-time using --ldflags -X
-var Version = "(unknown version)"
-
 func noop(_ context.Context) {}
 
 // Builtin for showing this image's version.
-func showVersion(ctx context.Context, Version string, args ...string) error {
-	fmt.Printf("Version %s\n", Version)
+func showVersion(ctx context.Context, version string, args ...string) error {
+	fmt.Printf("Version %s\n", version)
 
 	return nil
 }
 
 func main() {
-	// Allow ambassador.version to override the compiled-in Version.
-	//
-	// "Wait wait wait wait wait," I hear you cry. "Why in the world are you
-	// doing this??" Two reasons:
-	//
-	// 1. ambassador.version is updated during the RC and GA process to always
-	//    contain the Most Polite Version of the version number -- this is the
-	//    ONE thing that should be shown to users.
-	// 2. We do _not_ recompile busyambassador during the RC and GA process, and
-	//    we don't want to: we want to ship the bits we tested, and while we're
-	//    OK with altering a text file after that, recompiling feels weirder. So
-	//    we don't.
+	// The version number is set at run-time by reading the 'ambassador.version' file.  We do
+	// this instead of compiling in a version so that we can promote RC images to GA without
+	// recompiling anything.
 	//
-	// End result: fall back on the compiled-in version, but let ambassador.version
-	// be the primary.
-
-	// THIS IS A CLOSURE CALL, not just an anonymous function definition. Making the
-	// call lets me defer file.Close().
-	func() {
-		file, err := os.Open("/buildroot/ambassador/python/ambassador.version")
-
-		if err != nil {
-			// We DON'T log errors here; we just silently fall back to the
-			// compiled-in version. This is because the code in main() here is
-			// running _wicked_ early, and logging setup happens _after_ this
-			// function.
-			//
-			// XXX Letting the logging setup happen here, instead, would likely
-			// be an improvement.
-			return
-		}
-
-		defer file.Close()
-
-		// Read line by line and hunt for BUILD_VERSION.
-		scanner := bufio.NewScanner(file)
-
-		for scanner.Scan() {
-			line := scanner.Text()
-
+	// Keep this parsing logic in-sync with VERSION.py.
+	version := "dirty"
+	if verBytes, err := os.ReadFile("/buildroot/ambassador/python/ambassador.version"); err == nil {
+		verLines := strings.Split(string(verBytes), "\n")
+		for _, line := range verLines {
 			if strings.HasPrefix(line, "BUILD_VERSION=") {
-				// The BUILD_VERSION line should be e.g.
-				//
-				// BUILD_VERSION="2.0.4-rc.2"
-				//
-				// so... cheat. Split on " and look for the second field.
-				v := strings.Split(line, "\"")
-
-				// If we don't get exactly three fields, though, something
-				// is wrong and we'll give up.
-				if len(v) == 3 {
-					Version = v[1]
+				vals, err := shellquote.Split(strings.TrimPrefix(line, "BUILD_VERSION="))
+				if err == nil && len(vals) > 0 {
+					version = vals[0]
 				}
-				// See comments toward the top of this function for why there's no
-				// logging here.
-				// else {
-				// 	fmt.Printf("VERSION OVERRIDE: got %#v ?", v)
-				// }
 			}
 		}
+	}
 
-		// Again, see comments toward the top of this function for why there's no
-		// logging here.
-		// if err := scanner.Err(); err != nil {
-		// 	fmt.Printf("VERSION OVERRIDE: scanner error %s", err)
-		// }
-	}()
-
-	busy.Main("busyambassador", "Ambassador", Version, map[string]busy.Command{
+	busy.Main("busyambassador", "Ambassador", version, map[string]busy.Command{
 		"ambex":      {Setup: environment.EnvironmentSetupEntrypoint, Run: ambex.Main},
 		"kubestatus": {Setup: environment.EnvironmentSetupEntrypoint, Run: kubestatus.Main},
 		"entrypoint": {Setup: noop, Run: entrypoint.Main},
 		"reproducer": {Setup: noop, Run: reproducer.Main},
-		"agent":      {Setup: environment.EnvironmentSetupEntrypoint, Run: amb_agent.Main},
+		"agent":      {Setup: environment.EnvironmentSetupEntrypoint, Run: agent.Main},
 		"version":    {Setup: noop, Run: showVersion},
 		"apiext":     {Setup: noop, Run: apiext.Main},
 	})

python/ambassador/VERSION.py

@@ -16,6 +16,7 @@
 from typing import NamedTuple
 
 try:
+    # Keep this in-sync with cmd/busyambassador/main.go.
     with open(os.path.join(os.path.dirname(__file__), "..", "ambassador.version")) as version:
         exec(version.read())
 except FileNotFoundError: