Skip to content

OCPBUGS-23055: bugfix for operator-controller not populating commit info #376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 24, 2025
Merged

OCPBUGS-23055: bugfix for operator-controller not populating commit info #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea7eb1c
UPSTREAM: <carry>: set GIT_COMMIT env from SOURCE_GIT_COMMIT in Docke…
rashmigottipati Jun 6, 2025
8ae090d
UPSTREAM: <carry>: add openshift specific build target to pass commit…
ankitathomas Jun 11, 2025
e29a35b
UPSTREAM: <carry>: add source commit into binaries when linking
bentito Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,10 @@ E2E_REGISTRY_IMAGE=localhost/e2e-test-registry:devel
image-registry: export GOOS=linux
image-registry: export GOARCH=amd64
image-registry: ## Build the testdata catalog used for e2e tests and push it to the image registry
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/push/bin/push ./testdata/push/push.go
# Use double quotes (not single quotes) for build flags like -ldflags, -tags, etc.
# Single quotes are passed literally and not stripped by `go build`, which can cause errors
# or inject unintended characters into the binary (e.g., version metadata).
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags "$(GO_BUILD_LDFLAGS)" -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/push/bin/push ./testdata/push/push.go
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags "$(GO_BUILD_LDFLAGS)" -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/push/bin/push ./testdata/push/push.go
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) \
-tags "$(GO_BUILD_TAGS)" \
# Use double quotes (not single quotes) for build flags like -ldflags, -tags, etc.
# Single quotes are passed literally and not stripped by `go build`, which can cause errors
# or inject unintended characters into the binary (e.g., version metadata).
-ldflags "$(GO_BUILD_LDFLAGS)" \
-gcflags '$(GO_BUILD_GCFLAGS)' \
-asmflags '$(GO_BUILD_ASMFLAGS)' \
-o ./testdata/push/bin/push \
./testdata/push/push.go

IMO: It seems better adding the inline comment — this is a subtle detail that can easily break things and go unnoticed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can have the comment here too, can't hurt. Changing in next commit

$(CONTAINER_RUNTIME) build -f ./testdata/Dockerfile -t $(E2E_REGISTRY_IMAGE) ./testdata
$(CONTAINER_RUNTIME) save $(E2E_REGISTRY_IMAGE) | $(KIND) load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME)
./testdata/build-test-registry.sh $(E2E_REGISTRY_NAMESPACE) $(E2E_REGISTRY_NAME) $(E2E_REGISTRY_IMAGE)
Expand Down Expand Up @@ -374,13 +377,16 @@ export GO_BUILD_GCFLAGS := all=-trimpath=$(PWD)
export GO_BUILD_EXTRA_FLAGS :=
export GO_BUILD_LDFLAGS := -s -w \
-X '$(VERSION_PATH).version=$(VERSION)' \
-X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)' \
-X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)'

BINARIES=operator-controller catalogd

.PHONY: $(BINARIES)
$(BINARIES):
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o $(BUILDBIN)/$@ ./cmd/$@
# use double quotes around $(GO_BUILD_LDFLAGS) to avoid conflicts with the
# single quotes that are embedded inside the variable itself. this prevents
# malformed arguments such as "malformed import path \" \"" when the git commit is empty.
go build $(GO_BUILD_FLAGS) $(GO_BUILD_EXTRA_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags "$(GO_BUILD_LDFLAGS)" -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o $(BUILDBIN)/$@ ./cmd/$@
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:
Could we format to make simple understand
And add the comment on top of the scenario wiith ""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reformated a bit, hopefully better now. see next commit


.PHONY: build-deps
build-deps: manifests generate fmt
Expand Down
37 changes: 32 additions & 5 deletions internal/shared/version/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package version
import (
"fmt"
"runtime/debug"
"strings"
)

var (
Expand All @@ -17,8 +18,32 @@ var (
}
)

// isUnset returns true when the provided string should be treated as an
// "unset" value. Builds that inject ldflags such as "-X var=" will set the
// variable to the empty string, which previously prevented the runtime build
// information gathered via debug.ReadBuildInfo from populating the field. For
// the purposes of version reporting we treat both the empty string and the
// literal "unknown" as unset.
func isUnset(s string) bool {
// trim any surrounding whitespace to ensure accurate unset detection
s = strings.TrimSpace(s)
return s == "" || s == "unknown"
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string will be used many times, could we create a const with this value?
also, i think it would be more robust like

func isUnset(s string) bool {
    s = strings.TrimSpace(s)
    return s == "" || s == unknown
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, Per noted too, changing in next commit

}

func String() string {
return fmt.Sprintf("version: %q, commit: %q, date: %q, state: %q", version, gitCommit, commitDate, repoState)
return fmt.Sprintf("version: %q, commit: %q, date: %q, state: %q",
valueOrUnknown(version),
valueOrUnknown(gitCommit),
valueOrUnknown(commitDate),
valueOrUnknown(repoState))
}

func valueOrUnknown(v string) string {
v = strings.TrimSpace(v)
if v == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we strings.TrimSpace here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, isUnset now trims whitespace in the next commit. I made same change in valueOrUnknown, seemed like the thing to do.

return "unknown"
}
return v
}

func init() {
Expand All @@ -29,18 +54,20 @@ func init() {
for _, setting := range info.Settings {
switch setting.Key {
case "vcs.revision":
if gitCommit == "unknown" {
if isUnset(gitCommit) {
gitCommit = setting.Value
}
case "vcs.time":
commitDate = setting.Value
if isUnset(commitDate) {
commitDate = setting.Value
}
case "vcs.modified":
if v, ok := stateMap[setting.Value]; ok {
if v, ok := stateMap[setting.Value]; ok && isUnset(repoState) {
repoState = v
}
}
}
if version == "unknown" {
if isUnset(version) && info.Main.Version != "" {
version = info.Main.Version
}
}
14 changes: 14 additions & 0 deletions openshift/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ include $(addprefix $(DIR)/vendor/github.com/openshift/build-machinery-go/make/,

include $(DIR)/.bingo/Variables.mk

# Prefer the upstream source commit that the Dockerfile passes in via
# `ENV GIT_COMMIT=<sha>`. If that variable is not already defined fall back to
# the commit recorded by the OpenShift image build pipeline.
ifeq ($(origin GIT_COMMIT), undefined)
GIT_COMMIT := $(OPENSHIFT_BUILD_COMMIT) # populated by OpenShift build machinery
endif
export GIT_COMMIT
VERSION_PATH := github.com/operator-framework/operator-controller/internal/shared/version
export GO_BUILD_LDFLAGS := -s -w -X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)'

.PHONY: verify
verify: ## Run downstream-specific verify
$(MAKE) tidy fmt generate -C $(DIR)/../
Expand Down Expand Up @@ -45,3 +55,7 @@ test-experimental-e2e: ## Run the experimental e2e tests.
$(DIR)/operator-controller/build-test-registry.sh $(E2E_REGISTRY_NAMESPACE) $(E2E_REGISTRY_NAME) $(E2E_REGISTRY_IMAGE)
cd $(DIR)/../; \
go test $(DOWNSTREAM_EXPERIMENTAL_E2E_FLAGS) ./test/experimental-e2e/...;

PHONY: go-build-local
go-build-local:
$(MAKE) -f Makefile go-build-local
3 changes: 3 additions & 0 deletions openshift/catalogd.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.24-openshift-4.20 AS builder

ARG SOURCE_GIT_COMMIT
ENV GIT_COMMIT=${SOURCE_GIT_COMMIT}
WORKDIR /build
COPY . .
RUN make go-build-local
Expand Down
5 changes: 4 additions & 1 deletion openshift/operator-controller.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.24-openshift-4.20 AS builder

ARG SOURCE_GIT_COMMIT
ENV GIT_COMMIT=${SOURCE_GIT_COMMIT}
WORKDIR /build
COPY . .
RUN make go-build-local && \
RUN make -f openshift/Makefile go-build-local && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to make this change to sort out the issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the extra “-f openshift/Makefile” is necessary

  • Two Makefiles exist

    • the project-wide Makefile at repo root (upstream build)
    • openshift/Makefile (downstream build wrapper) that includes build-machinery-go and re-exports targets with OCP-specific flags.

  • openshift/Makefile injects downstream-required build flags

    • sets GO_BUILD_LDFLAGS to embed the commit that the OpenShift build pipeline exports (OPENSHIFT_BUILD_COMMIT)
    • imports build-machinery-go targets needed by CI images
    • wraps the “upstream go-build-local” target so the binary is built exactly the way OCP expects.

  • The Docker build runs from repo root (WORKDIR /build).
    If we just run “make go-build-local” the root Makefile would be picked up and none of the downstream flags / targets above would be applied, giving us a binary that is missing version metadata and may fail downstream CI validations.

  • Adding “-f openshift/Makefile” therefore guarantees that the correct Makefile is used regardless of where the docker build is invoked and keeps the container image build reproducible in both OpenShift CI and local development.

Because the build currently works only with the downstream Makefile, the flag is not optional; removing it would silently switch us back to the upstream build path and drop the commit/version injection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have removed this question. I put it first in the review, and then, after navigating, I got it.
Thank you a lot. 🙏

# Build the OLMv1 Test Extension binary.
# This is used by openshift/origin to allow us to register the OLMv1 test extension
# The binary needs to be added in the component image and OCP image
Expand Down
3 changes: 3 additions & 0 deletions openshift/registry.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.24-openshift-4.20 AS builder

ARG SOURCE_GIT_COMMIT
ENV GIT_COMMIT=${SOURCE_GIT_COMMIT}
WORKDIR /build
COPY . .
# TODO Modify upstream Makefile to separate the 'go build' commands
Expand Down