Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make specs-go a module #170

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Oct 26, 2023

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 bart0sh force-pushed the PR015-add-specs-go-mod branch 2 times, most recently from 3e427f2 to dbb8234 Compare October 26, 2023 11:13
go.sum Outdated
@@ -13,6 +13,7 @@ github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9
github.com/mndrix/tap-go v0.0.0-20171203230836-629fa407e90b/go.mod h1:pzzDgJWZ34fGzaAZGFW22KVZDfyrYW+QABMrWnJBnSs=
github.com/mrunalp/fileutils v0.5.0/go.mod h1:M1WthSahJixYnrXQl/DFQuteStB1weuxD2QJNHXfbSQ=
github.com/opencontainers/runtime-spec v1.0.3-0.20220825212826-86290f6a00fb/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
github.com/opencontainers/runtime-spec v1.0.3-0.20220909204839-494a5a6aca78/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: this is the version used in k/k: https://github.com/kubernetes/kubernetes/blob/v1.29.0-alpha.2/go.mod#L206
I guess a lot of deps were removed from cmd/cdi/go.sum because of this change, which looks rather good.

@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 26, 2023

/assign @elezar @klihub @kad @pohly

@elezar
Copy link
Contributor

elezar commented Oct 26, 2023

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
Copy link
Contributor

klihub commented Oct 26, 2023

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

@klihub
Copy link
Contributor

klihub commented Oct 26, 2023

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...

Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

@bart0sh I'm fine with turning this into a sub-module of its own.

@klihub
Copy link
Contributor

klihub commented Oct 26, 2023

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
Copy link
Contributor Author

bart0sh commented Oct 27, 2023

@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
Copy link
Contributor Author

bart0sh commented Oct 27, 2023

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

@klihub
Copy link
Contributor

klihub commented Oct 27, 2023

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

@bart0sh You mean this one or #171 itself ?

@klihub klihub self-requested a review October 27, 2023 12:17
Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub requested a review from elezar October 27, 2023 12:17
@elezar
Copy link
Contributor

elezar commented Oct 27, 2023

@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
Copy link
Contributor

pohly commented Oct 27, 2023

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

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

@elezar
Copy link
Contributor

elezar commented Oct 27, 2023

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
Copy link
Contributor

pohly commented Oct 27, 2023

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".

specs-go/go.mod Outdated Show resolved Hide resolved
@klihub klihub self-requested a review October 27, 2023 14:23
Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

@bart0sh Could you fix the go.mod thinko @pohly spotted ?

@bart0sh bart0sh force-pushed the PR015-add-specs-go-mod branch from dbb8234 to 3a75cb1 Compare October 27, 2023 15:24
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 27, 2023

@bart0sh You mean this one or #171 itself ?

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

@klihub klihub requested review from pohly and klihub October 27, 2023 16:11
bart0sh and others added 2 commits October 30, 2023 11:43
This should allow to import CDI spec structures without adding
a lot of CDI runtime deps.

It should partly fix cncf-tags#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:
- this can be replaced by the specs-go import:
  https://github.com/kubernetes/kubernetes/blob/master/test/e2e/dra/test-driver/app/cdi.go
- this can import specs-go and use JSON marshalling instead of
  quite ugly approach it uses to write CDI files:
  https://github.com/kubernetes/kubernetes/blob/master/test/images/sample-device-plugin/sampledeviceplugin.go#L237

Signed-off-by: Ed Bartosh <[email protected]>
@elezar elezar force-pushed the PR015-add-specs-go-mod branch from 3a75cb1 to 85e2e5b Compare October 30, 2023 10:50
@elezar
Copy link
Contributor

elezar commented Oct 30, 2023

/lgtm

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

@elezar elezar merged commit 8ef149a into cncf-tags:main Oct 30, 2023
7 checks passed
@elezar elezar mentioned this pull request Nov 4, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reconsider dependencies
4 participants