[th/make-generate-tidy] makefile: run go mod tidy during make generate

[thom311]

We have multiple go.mod files, this does not seem to agree with operator-sdk. operator-sdk will generate files and call make generate. However, that will then fail because we didn't call go mod tidy for api/ directory.

Note that compared to sriov-network-operator, in dpu-operator we have several go.mod files. That does not seem to agree with operator-sdk, or at least, something is not hooked up right.

Hack make generate to call go mod tidy first.

Otherwise:

  $ git checkout -B C origin/main && \
    git reset --hard HEAD && \
    git clean -fdx && \
    make operator-sdk && \
    ./bin/operator-sdk create webhook --group config --version v1 --kind DpuOperatorConfig --defaulting --programmatic-validation
  branch 'C' set up to track 'origin/main'.
  Reset branch 'C'
  Your branch is up to date with 'origin/main'.
  HEAD is now at e58a9c9c830a Merge pull request #194 from thom311/th/hack-scripts-cleanup
  INFO[0000] Writing kustomize manifests for you to edit...
  ERRO[0000] Unable to find the target #- path: manager_webhook_patch.yaml to uncomment in the file config/default/kustomization.yaml.
  INFO[0000] Writing scaffold for you to edit...
  INFO[0000] api/v1/dpuoperatorconfig_webhook.go
  INFO[0000] api/v1/dpuoperatorconfig_webhook_test.go
  INFO[0000] api/v1/webhook_suite_test.go
  INFO[0000] Update dependencies:
  $ go mod tidy
  INFO[0000] Running make:
  $ make generate
  test -s /data/src/dpu-operator/bin/controller-gen && /data/src/dpu-operator/bin/controller-gen --version | grep -q v0.15.0 || \
  GOBIN=/data/src/dpu-operator/bin GOFLAGS='' go install sigs.k8s.io/controller-tools/cmd/[email protected]
  GOFLAGS='' /data/src/dpu-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
  Error: load packages in root "/data/src/dpu-operator/api": err: exit status 1: stderr: go: updates to go.mod needed; to update it:
          go mod tidy

[openshift-ci]

Hi @thom311. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[bn222]

/hold

Doesn't this behave the same way if we didn't have a go.mod?

[thom311]

Doesn't this behave the same way if we didn't have a go.mod?

This patch has only an effect, when make generate is called.

I don't understand what k8s' make generate is supposed to do. So the answer depends on that.

[bn222]

I don't think we want to mix make generate with go mod tidy. I think go mod tidy should be run separately.

[thom311]

I don't think we want to mix make generate with go mod tidy. I think go mod tidy should be run separately.

There is a reproducer in the commit message that you can just run.

How do you suggest to solve this differently? Especially seeing that make generate is not run by me directly, but by operator-sdk indirectly?

[bn222]

It think the right place is make vendor , and we also need to extend make vendor-check

[bn222]

/unhold
/ok-to-test

[thom311]

Note that the build target also includes the generate target (build is also the default target). That means, a plain make or make build now also runs go mod tidy. That seems wrong that a build tries to generate/tidy the source files.

But I think the problem of that is that it's wrong that build does a generate. The problem is not that generate should not do a go mod tidy.

[bn222]

/test make-e2e-test

[openshift-ci-robot]

/test remaining-required

[openshift-ci-robot]

/test remaining-required

[vrindle]

/test make-e2e-test

[openshift-ci]

@thom311: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/make-fast-e2e-test	3a8b7b4	link	true	/test make-fast-e2e-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[thom311]

See similar comment from @bn222 here:

This prevents separate commits with vendor

That part is now dropped. make generate will no longer also depend on vendor target. Which defeats most of the purpose of why this commit was done. However, that part will temporarily come back in #213.

[bn222]

/lgtm
/approve
/ok-to-test

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bn222, thom311

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [bn222]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

/test remaining-required

[openshift-bot]

[ART PR BUILD NOTIFIER]

Distgit: dpu-operator
This PR has been included in build dpu-operator-container-v4.19.0-202411212037.p0.g99f3e30.assembly.stream.el9.
All builds following this will include this PR.

[openshift-bot]

[ART PR BUILD NOTIFIER]

Distgit: dpu-cni
This PR has been included in build dpu-cni-container-v4.19.0-202411212037.p0.g99f3e30.assembly.stream.el9.
All builds following this will include this PR.

[openshift-bot]

[ART PR BUILD NOTIFIER]

Distgit: dpu-daemon
This PR has been included in build dpu-daemon-container-v4.19.0-202411212037.p0.g99f3e30.assembly.stream.el9.
All builds following this will include this PR.