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

Use current fsnotify library, drop viper dependency #95

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Nov 9, 2022

Working through the review of integrating this into kubernetes in kubernetes/kubernetes#111023, I found some dependency issues:

Signed-off-by: Jordan Liggitt <liggitt@google.com>
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
Copy link
Contributor

klihub commented Nov 11, 2022

Working through the review of integrating this into kubernetes in kubernetes/kubernetes#111023, I found some dependency issues:

Thanks ! Strange that none of us has spotted that mismatching extra fsnotify earlier. Definitely let's get rid of it. It wasn't deliberate at all.

That said, wouldn't it also make sense to get rid of cobra as a transitive dependency for cdi itself ? We should be able to do that easily by taking this one step further and turning cmd/cdi into a go module of it's own. So basically dropping something like this in cmd/cdi as go.mod:

module github.com/container-orchestrated-devices/container-device-interface/cmd/cdi

go 1.19

require (
        github.com/container-orchestrated-devices/container-device-interface v0.0.0
        github.com/fsnotify/fsnotify v1.5.1
        github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417
        github.com/opencontainers/runtime-tools v0.0.0-20190417131837-cd1349b7c47e
        github.com/pkg/errors v0.9.1
        github.com/spf13/cobra v1.6.0
        sigs.k8s.io/yaml v1.3.0
)

require (
        github.com/blang/semver v3.5.1+incompatible // indirect
        github.com/hashicorp/errwrap v1.0.0 // indirect
        github.com/hashicorp/go-multierror v1.1.1 // indirect
        github.com/inconshreveable/mousetrap v1.0.1 // indirect
        github.com/opencontainers/runc v1.1.2 // indirect
        github.com/opencontainers/selinux v1.10.0 // indirect
        github.com/sirupsen/logrus v1.8.1 // indirect
        github.com/spf13/pflag v1.0.5 // indirect
        github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635 // indirect
        github.com/xeipuuv/gojsonpointer v0.0.0-20180127040702-4e3ac2762d5f // indirect
        github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
        github.com/xeipuuv/gojsonschema v1.2.0 // indirect
        golang.org/x/sys v0.0.0-20211116061358-0a5406a5449c // indirect
        gopkg.in/yaml.v2 v2.4.0 // indirect
)

replace github.com/container-orchestrated-devices/container-device-interface => ../..

and maybe also updating the related Makefile rules with something like this:

diff --git a/Makefile b/Makefile
index 59a6b5d..775b3d5 100644
--- a/Makefile
+++ b/Makefile
@@ -49,6 +49,10 @@ bin/%:
        $(Q)echo "Building $@..."; \
        $(GO_BUILD) -o $@ ./$(subst bin/,cmd/,$@)
 
+bin/cdi:
+       $(Q)echo "Building $@..."; \
+       cd cmd/cdi; $(GO_BUILD) -o $(abspath $@) .
+
 #
 # cleanup targets
 #
@@ -84,7 +88,7 @@ bin/validate: cmd/validate/validate.go $(wildcard schema/*.json)
 # quasi-automatic dependency for bin/cdi
 bin/cdi: $(wildcard cmd/cdi/*.go cmd/cdi/cmd/*.go) $(shell \
             for dir in \
-                $$($(GO_CMD) list -f '{{ join .Deps "\n"}}' ./cmd/cdi/... | \
+                $$(cd ./cmd/cdi; $(GO_CMD) list -f '{{ join .Deps "\n"}}' ./... | \
                       grep $(CDI_PKG)/pkg/ | \
                       sed 's:$(CDI_PKG):.:g'); do \
                 find $$dir -name \*.go; \

WDYT ?

@liggitt
Copy link
Contributor Author

liggitt commented Nov 11, 2022

I don't have context to know what the maintainers want to do with the cobra dep. Multi-module repos can be painful to manage, so I wouldn't assume folks here would want to do that.

I'd probably recommend merging this as a first easy step, then letting a module carve-out effort be done as a follow-up if desired

@klihub klihub merged commit 9805f6f into cncf-tags:main Nov 11, 2022
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.

2 participants