Makefile: Export BPFMAN_AGENT_IMG and BPFMAN_OPERATOR_IMG

[frobware]

Ensure BPFMAN_AGENT_IMG and BPFMAN_OPERATOR_IMG are exported so that they are available to child processes invoked by make. The Makefile provides helpful default values for these variables if not specified. However, without exporting them, you must either specify the values on the make invocation line or export them from your shell. This change resolves issues where integration tests fail due to these variables being missing from the environment.

Without these values being exported, we observe the following:

% make test-integration
go clean -testcache
cd config/bpfman-deployment && \
  sed -e 's@bpfman\.image=.*@bpfman.image=quay.io/bpfman/bpfman:latest@' \
      -e 's@bpfman\.agent\.image=.*@bpfman.agent.image=quay.io/bpfman/bpfman-agent:latest@' \
	  kustomization.yaml.env > kustomization.yaml
GOFLAGS="-tags=integration_tests" go test -race -v ./test/integration/...
BPFMAN_AGENT_IMG, and BPFMAN_OPERATOR_IMG must be provided
FAIL	github.com/bpfman/bpfman-operator/test/integration	0.075s
FAIL
make: *** [Makefile:290: test-integration] Error 1

And when they are exported, we observe the following:

% make test-integration
go clean -testcache
cd config/bpfman-deployment && \
  sed -e 's@bpfman\.image=.*@bpfman.image=quay.io/bpfman/bpfman:latest@' \
      -e 's@bpfman\.agent\.image=.*@bpfman.agent.image=quay.io/bpfman/bpfman-agent:latest@' \
	  kustomization.yaml.env > kustomization.yaml
GOFLAGS="-tags=integration_tests" go test -race -v ./test/integration/...
INFO: using bpfmanAgentImage=quay.io/bpfman/bpfman-agent:latest and bpfmanOperatorImage=quay.io/bpfman/bpfman-operator:latest
INFO: creating a new kind cluster
...

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.36%. Comparing base (bf7994e) to head (97119e5).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
- Coverage   27.39%   27.36%   -0.03%     
==========================================
  Files          81       81              
  Lines        6968     6975       +7     
==========================================
  Hits         1909     1909              
- Misses       4873     4880       +7     
  Partials      186      186              

Flag	Coverage Δ
unittests	27.36% <ø> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[msherif1234]

so with this change instead of passing the IMG to make intg-test u can assign the IMG 1st then do the make intg ? I think its kind of common practice with operator sdk setup to set the env var when invoking the make?

[frobware]

I think its kind of common practice with operator sdk setup to set the env var when invoking the make?

However, the current setup makes the easy path seem difficult:

$ git clone /path/to/repo
$ make integration-test

This doesn't work out of the box.

Is there any value in defaulting these variables in the Makefile if they don't get propagated to the binaries that depend on them?

You can still manually export the values in your shell:

$ export BPFMAN_AGENT_IMG=some-other-registry.com/bpfman/bpfman-agent:some-hash
$ make integration-test

Or specify them when invoking make:

$ make BPFMAN_AGENT_IMG=some-other-registry.com/bpfman/bpfman-agent:some-hash integration-test

These two methods will override the default values specified in the Makefile.

However, for a newcomer, running make integration-test should work out of the box.

[msherif1234]

maybe also worth exporting BPFMAN_IMG too? might be good idea adding both options in the readme for reference

[frobware]

maybe also worth exporting BPFMAN_IMG too? might be good idea adding both options in the readme for reference

BPFMAN_IMG doesn't appear to be used outside of the Makefile:

% ag BPFMAN_IMG
Makefile
50:BPFMAN_IMG ?= quay.io/bpfman/bpfman:$(IMAGE_TAG)
290:	  $(SED) -e 's@bpfman\.image=.*@bpfman.image=$(BPFMAN_IMG)@' \
303:	  $(SED) -e 's@bpfman\.image=.*@bpfman.image=$(BPFMAN_IMG)@' \
338:	$(OCI_BIN) push ${BPFMAN_IMG}
402:	 $(SED)  -e 's@bpfman\.image=.*@bpfman.image=$(BPFMAN_IMG)@' \
427:	  $(SED) -e 's@bpfman\.image=.*@bpfman.image=$(BPFMAN_IMG)@' \

Given that BPFMAN_IMG is not used outside of the Makefile, I don't think it should be exported.

[frobware]

might be good idea adding both options in the readme for reference

Should both options, as noted in this comment, be added to the top-level README.md or the output of make help? I'm not sure where the information should best sit

[msherif1234]

might be good idea adding both options in the readme for reference

Should both options, as noted in this comment, be added to the top-level README.md or the output of make help? I'm not sure where the information should best sit

readme should be sufficient Thank you!!

[anfredette]

@frobware thanks for doing this. It's good to have an easy path, and it looks good to me. We have a description of how to run the tests here which I think should be updated.

[frobware]

Actually, this still doesn't work out of the box. You need to make the following change if you just clone the repo and expect make test-integration to work:

% git diff
diff --git a/config/test/kustomization.yaml b/config/test/kustomization.yaml
index 5f52dab..51631c1 100644
--- a/config/test/kustomization.yaml
+++ b/config/test/kustomization.yaml
@@ -46,4 +46,4 @@ resources:
 images:
   - name: quay.io/bpfman/bpfman-operator
     newName: quay.io/bpfman/bpfman-operator
-    newTag: int-test
+    newTag: latest
diff --git a/config/test/patch.yaml b/config/test/patch.yaml
index 709b3cd..240b1ae 100644
--- a/config/test/patch.yaml
+++ b/config/test/patch.yaml
@@ -4,7 +4,7 @@ metadata:
   name: config
   namespace: kube-system
 data:
-  bpfman.agent.image: quay.io/bpfman/bpfman-agent:int-test
+  bpfman.agent.image: quay.io/bpfman/bpfman-agent:latest
   bpfman.image: quay.io/bpfman/bpfman:latest
   bpfman.log.level: bpfman=debug
   bpfman.agent.log.level: debug

[frobware]

/hold

Let's figure out how int-test is supposed to work.