ceph / ceph-csi-operator

Kubernetes operator for managing the CephCSI plugins
Apache License 2.0
16 stars 18 forks source link

make test fails on fresh clone #133

Closed obnoxxx closed 2 months ago

obnoxxx commented 2 months ago

Describe the bug

On my Fedora 40 system, make testfails with a panic on a fresh main clone:


$ make test
/home/obnox/go/src/github.com/ceph/ceph-csi-operator/bin/controller-gen-v0.14.0 rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
/home/obnox/go/src/github.com/ceph/ceph-csi-operator/bin/controller-gen-v0.14.0 object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
Downloading sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.17
go: downloading sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20240812162837-9557f1031fe4
go: downloading sigs.k8s.io/controller-runtime v0.17.6
go: downloading github.com/spf13/afero v1.6.0
go: downloading go.uber.org/zap v1.26.0
go: downloading sigs.k8s.io/yaml v1.3.0
go: downloading go.uber.org/multierr v1.10.0
KUBEBUILDER_ASSETS="/home/obnox/go/src/github.com/ceph/ceph-csi-operator/bin/k8s/1.29.0-linux-amd64" go test $(go list ./... | grep -v /e2e) -coverprofile cover.out
    github.com/ceph/ceph-csi-operator/cmd       coverage: 0.0% of statements
    github.com/ceph/ceph-csi-operator/test/utils        coverage: 0.0% of statements
    github.com/ceph/ceph-csi-operator/internal/utils        coverage: 0.0% of statements
panic: Required OPERATOR_NAMESPACE environment variable is either missing or empty

goroutine 1 [running]:
github.com/ceph/ceph-csi-operator/internal/controller.init.func1(...)
    /home/obnox/go/src/github.com/ceph/ceph-csi-operator/internal/controller/defaults.go:63
github.com/ceph/ceph-csi-operator/internal/utils.Call[...](...)
    /home/obnox/go/src/github.com/ceph/ceph-csi-operator/internal/utils/core.go:121
github.com/ceph/ceph-csi-operator/internal/controller.init()
    /home/obnox/go/src/github.com/ceph/ceph-csi-operator/internal/controller/defaults.go:60 +0x485
FAIL    github.com/ceph/ceph-csi-operator/internal/controller   0.020s
FAIL
make: *** [Makefile:114: test] Error 1
$ 

Environment details

Steps to reproduce

Steps to reproduce the behavior:

Here is what I did:

  1. set up golang/git dev environment on Fedora 40.
  2. git clone the ceph-csi-operator repo
  3. run make (--> succeeds)
  4. run make test
  5. See error

Actual results

Describe what happened

make testfails with the panic pasted above

Expected behavior

make testshould pass.

Logs

N/A (see paste above)

Additional context

nb-ohad commented 2 months ago

@obnoxxx The behavior of not allowing the operator to run (on production) if the namespace env var is not defined is by design. Locally, when I run tests for this operator I use the command: OPERATOR_NAMESPACE="...." make test

After talking with @Madhu-1, we agreed that we want to preserve the production behavior while making the test usage simpler. For that, I am proposing the following fix: A change to the make test target to set a default value for the OPERATOR_NAMESPACE, only if it is not defined. I would further suggest using the same default namespace we use for the make build-installer and make deploy commands as the default value.

This change should be local to the test target and should not have any side effects on other make targets.

Madhu-1 commented 2 months ago

+1

JFYI:- even after the change, the test won't run it fails in the next steps.

obnoxxx commented 2 months ago

@Madhu-1 wrote:

JFYI:- even after the change, the test won't run it fails in the next steps.

I can confirm this:

Running OPERATOR_NAMESPACE=foo make test fails, but differently, without the panic.

@Madhu-1 tells me, however, that there is no point in fixing the test currently, as there are plans to remove/replace the whole test code.

Is there an issue to track that?

Madhu-1 commented 2 months ago

@nb-ohad is working on the test suite for it, AFAIK we don't have an issue tracker for it.

nb-ohad commented 2 months ago

@obnoxxx The fix for the make test to introduce a default namespace is necessary even with my changes to how we implement tests.

The second issue, you might want to skip that one

obnoxxx commented 2 months ago

@nb-ohad wrote:

@obnoxxx The fix for the make test to introduce a default namespace is necessary even with my changes to how we implement tests.

The second issue, you might want to skip that one

Are you suggesting, in order to address this issue, to fix the missing namespace setting and ignore the other test failure?

Madhu-1 commented 2 months ago

@nb-ohad wrote:

@obnoxxx The fix for the make test to introduce a default namespace is necessary even with my changes to how we implement tests. The second issue, you might want to skip that one

Are you suggesting, in order to address this issue, to fix the missing namespace setting and ignore the other test failure?

Yes that's correct