cncf-tags / container-device-interface

Apache License 2.0
190 stars 36 forks source link

make specs-go a module #170

Closed bart0sh closed 8 months ago

bart0sh commented 8 months ago

This should allow to import CDI spec structures without adding a lot of CDI runtime deps.

It should partly fix #97 and allow Kubernetes to refer this module directly. There are at least 2 places in the k/k codebase that would benefit from this change:

bart0sh commented 8 months ago

/assign @elezar @klihub @kad @pohly

elezar commented 8 months ago

One question: Does this mean we need to update the mod-tidy and mod-verify targets in the Makefile to include the specs-go folder too?

klihub commented 8 months ago

So I guess after merging this we should start tagging also the specs-go golang sub-module ?

klihub commented 8 months ago

One question: Does this mean we need to update the mod-tidy and mod-verify targets in the Makefile to include the specs-go folder too?

Yes, I think so. Maybe we should replace the current, manually spelt out tidying and verifying rules with something more simple and automatic like, for instance:

diff --git a/Makefile b/Makefile
index 6fc1836..9f70ee7 100644
--- a/Makefile
+++ b/Makefile
@@ -64,30 +64,18 @@ $(BINARIES): bin/%:
 #
 # go module tidy and verify targets
 #
-.PHONY: mod-tidy $(CMD_MOD_TIDY_TARGETS) mod-tidy-root
-.PHONY: mod-verify $(CMD_MOD_VERIFY_TARGETS) mod-verify-root
-
-CMD_MOD_TIDY_TARGETS := mod-tidy-cdi mod-tidy-validate
-CMD_MOD_VERIFY_TARGETS := mod-verify-cdi mod-verify-validate
-
-mod-tidy-root:
-       $(Q)echo "Running $@..."; \
-       $(GO_CMD) mod tidy
-
-$(CMD_MOD_TIDY_TARGETS): mod-tidy-%: mod-tidy-root
-       $(Q)echo "Running $@... in $(abspath ./cmd/$(*))"; \
-       (cd $(abspath ./cmd/$(*)) && $(GO_CMD) mod tidy)
-
-mod-verify-root: mod-tidy-root
-       $(Q)echo "Running $@..."; \
-       $(GO_CMD) mod verify
-
-$(CMD_MOD_VERIFY_TARGETS): mod-verify-%: mod-tidy-% mod-verify-root
-       $(Q)echo "Running $@... in $(abspath ./cmd/$(*))"; \
-       (cd $(abspath ./cmd/$(*)) && pwd && $(GO_CMD) mod verify)
-
-mod-verify: $(CMD_MOD_VERIFY_TARGETS)
-mod-tidy: $(CMD_MOD_TIDY_TARGETS)
+.PHONY: mod-tidy
+.PHONY: mod-verify
+
+mod-tidy:
+       $(Q)for mod in $$(find . -name go.mod); do \
+           (echo "Tidying $$mod..."; cd $$(dirname $$mod) && go mod tidy) || exit 1; \
+       done
+
+mod-verify:
+       $(Q)for mod in $$(find . -name go.mod); do \
+           (echo "Verifying $$mod..."; cd $$(dirname $$mod) && go mod verify) || exit 1; \
+       done

 #
 # cleanup targets

And in addition to doing so, I would also add github workflow job to reject any PR where a make mod-tidy would introduce some changes...

@elezar @bart0sh If you guys agree, I can file a PR to these effects...

klihub commented 8 months ago

And in addition to doing so, I would also add github workflow job to reject any PR where a make mod-tidy would introduce some changes...

@elezar @bart0sh If you guys agree, I can file a PR to these effects...

@elezar @bart0sh #171

bart0sh commented 8 months ago

@klihub

So I guess after merging this we should start tagging also the specs-go golang sub-module ?

That would make sense from my point of view.

bart0sh commented 8 months ago

@klihub @elezar considering #171 lgtm/approve anyone?

klihub commented 8 months ago

@klihub @elezar considering #171 lgtm/approve anyone?

@bart0sh You mean this one or #171 itself ?

elezar commented 8 months ago

@klihub

So I guess after merging this we should start tagging also the specs-go golang sub-module ?

That would make sense from my point of view.

What does "tagging the sub-module" mean in this context. We only have a single tag associated with this repository.

pohly commented 8 months ago

What does "tagging the sub-module" mean in this context.

https://go.dev/doc/modules/managing-source#multiple-module-source

elezar commented 8 months ago

What does "tagging the sub-module" mean in this context.

https://go.dev/doc/modules/managing-source#multiple-module-source

OK. So if we create a git tag: specs-go/v0.6.0 this will effectively version the specs-go submodule and allow consumers to import this? Is importing from the root still supported, or would this change require that our consumers need to be updated?

pohly commented 8 months ago

I think consumers continue to import "github.com/container-orchestrated-devices/container-device-interface/specs-go", it just will have a different version than "github.com/container-orchestrated-devices/container-device-interface".

bart0sh commented 8 months ago

@bart0sh You mean this one or #171 itself ?

this one. I don't mind merging #171 before this or after.

elezar commented 8 months ago

/lgtm

@bart0sh for your info. I rebased on main and fixed the module definition for cmd/validate.