-
Notifications
You must be signed in to change notification settings - Fork 64
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
The knative.dev/hack
go package shouldn't contain any go deps
#318
Comments
knative.dev/hack
go package shouldn't contain any go deps
My ideas on fixing this aren't as simple as it seems at first:
move the
|
if err := copyDir(l, hack.Scripts, hackRootDir, "."); err != nil { |
So, most of the code can be moved, but still the main() func must be kept here.
create an additional go.mod
in cmd
This idea is also problematic. We can consolidate the code of the cmd/script
withing cmd
go submodule, and add the cmd
to the go.work
file, however we still need to reference the embed.go in the main dir. This can be done with:
require knative.dev/hack v0.0.0
replace knative.dev/hack => ../
But, by doing so, we would break the intended go run ...
command, as those can't use any replace
stanza in order to work.
BTW. We tried doing exactly the same thing with kntest from toolbox repo, which broke it (see the stale module: https://pkg.go.dev/knative.dev/toolbox/kntest - it didn't revert even after we did knative/toolbox#9)
/cc @dprotaso |
Maybe the only viable way would be to refactor the |
I don't know if I see the problem with the moving of the cmd/script @cardil. There is an assumption that there will be shell files I understand. However, invoking this would be from a place that we can populate with the shell files or maybe even provide a path arg? |
No. This can't be done like that. I described that in the comment above: #318 (comment) TLDR: The |
The knative.dev/hack go package shouldn't contain any go deps because it is being sourced in all repos in
hack/tools.go
file, like seen below. Having deps in this package could influence the deps in other consuming repos unintentionally.Caused by #222
More info #317 (comment)
/kind bug
The text was updated successfully, but these errors were encountered: