Skip to content

Add api generation in make generate#877

Closed
kashifest wants to merge 1 commit intometal3-io:masterfrom
Nordix:fix/go-mod-apis-kashif
Closed

Add api generation in make generate#877
kashifest wants to merge 1 commit intometal3-io:masterfrom
Nordix:fix/go-mod-apis-kashif

Conversation

@kashifest
Copy link
Copy Markdown
Member

@kashifest kashifest commented May 12, 2021

This PR adds api generation in generate Make target

Fixes #875

@metal3-io-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kashifest
To complete the pull request process, please assign maelk after the PR has been reviewed.
You can assign the PR to them by writing /assign @maelk in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@metal3-io-bot metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 12, 2021
@kashifest kashifest force-pushed the fix/go-mod-apis-kashif branch from b31001f to bbedcab Compare May 12, 2021 08:22
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 12, 2021
@kashifest kashifest force-pushed the fix/go-mod-apis-kashif branch 2 times, most recently from f08c8f2 to b25e297 Compare May 12, 2021 08:32
@kashifest
Copy link
Copy Markdown
Member Author

/test-integration
/cc @dhellmann

@metal3-io-bot metal3-io-bot requested a review from dhellmann May 18, 2021 18:53
@andfasano
Copy link
Copy Markdown
Member

I think that this could be a reasonable approach until we'll keep the two go.mod separated. Probably not something that will scale well, but for now seems enough.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2021
@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented May 20, 2021

@andfasano But this brings new dep for API sigs.k8s.io/controller-tools v0.5.0
And it is only needed for code-generating, effectively this makes separate go.mod useless, external projects would need to import a lot of unneeded dependencies just like it was before separate go.mod.

Also note that version of controller tools are different
https://github.com/metal3-io/baremetal-operator/blob/master/go.mod#L19

v0.5.0 VS v0.4.1 in core go.mod

I do not think that this is acceptable, controller-tools version should be same across whole repo

@kashifest
Copy link
Copy Markdown
Member Author

kashifest commented May 20, 2021

Also note that version of controller tools are different
https://github.com/metal3-io/baremetal-operator/blob/master/go.mod#L19

v0.5.0 VS v0.4.1 in core go.mod

I do not think that this is acceptable, controller-tools version should be same across whole repo

I can change that

@kashifest kashifest force-pushed the fix/go-mod-apis-kashif branch from b25e297 to 967b3fb Compare May 20, 2021 12:12
@metal3-io-bot
Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label May 20, 2021
@kashifest kashifest force-pushed the fix/go-mod-apis-kashif branch 2 times, most recently from 749d4ed to 89ec3c2 Compare May 20, 2021 12:31
@metal3-io-bot metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 20, 2021
@kashifest kashifest force-pushed the fix/go-mod-apis-kashif branch from 89ec3c2 to 849a03a Compare May 20, 2021 12:38
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 20, 2021
@kashifest kashifest force-pushed the fix/go-mod-apis-kashif branch from 849a03a to 74d6219 Compare May 20, 2021 12:41
@kashifest
Copy link
Copy Markdown
Member Author

/test-integration
@s3rj1k @andfasano PTAL again. I think we can take it in to fix apis generation until we have an alternative.

Comment thread Makefile
@@ -131,11 +131,13 @@ deploy: manifests ## Deploy controller in the configured Kubernetes cluster in ~
.PHONY: manifests
manifests: ## Generate manifests e.g. CRD, RBAC etc.
$(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AFAIK everything we need to generate is always in the apis directory, isn't it? I think we could remove this line (and the equivalent below).
That would at least solve the problem of trying to keep the controller-gen version the same in two different go.mod files.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

makes sense. @s3rj1k WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@zaneb
Copy link
Copy Markdown
Member

zaneb commented May 21, 2021

I opened #888 to do this without having to add dependencies to the apis module, so closing in favour of that per Kashif's comment #888 (comment).
/close

@metal3-io-bot
Copy link
Copy Markdown
Contributor

@zaneb: Closed this PR.

Details

In response to this:

I opened #888 to do this without having to add dependencies to the apis module, so closing in favour of that per Kashif's comment #888 (comment).
/close

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gomod for public API broke controller-tools object generation

5 participants