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

The knative.dev/hack go package shouldn't contain any go deps #318

Closed
Tracked by #134
cardil opened this issue Sep 21, 2023 · 5 comments · Fixed by #320
Closed
Tracked by #134

The knative.dev/hack go package shouldn't contain any go deps #318

cardil opened this issue Sep 21, 2023 · 5 comments · Fixed by #320
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@cardil
Copy link
Contributor

cardil commented Sep 21, 2023

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.

import (
  _ "knative.dev/hack"
)

Caused by #222
More info #317 (comment)

/kind bug

@knative-prow knative-prow bot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 21, 2023
@cardil cardil changed the title The knative.dev/hack go package shouldn't contain any go deps The knative.dev/hack go package shouldn't contain any go deps Sep 21, 2023
@cardil
Copy link
Contributor Author

cardil commented Sep 21, 2023

My ideas on fixing this aren't as simple as it seems at first:

@cardil: We could move the cmd/script to the toolbox, or just create an additional go.mod in cmd.

move the cmd/script to the toolbox

This can be done, but not all the code can be moved. The idea was that the script will be used like go run knative.dev/cmd/script library.sh, and that's why it embeds the *.sh files in the main module, see:

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)

@cardil
Copy link
Contributor Author

cardil commented Sep 21, 2023

/cc @dprotaso

@cardil
Copy link
Contributor Author

cardil commented Sep 21, 2023

Maybe the only viable way would be to refactor the cmd/script so it will not use any external deps, just built-in go packages?!?

@cardil cardil self-assigned this Sep 22, 2023
@krsna-m
Copy link
Contributor

krsna-m commented Sep 22, 2023

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?

@cardil
Copy link
Contributor Author

cardil commented Sep 22, 2023

@kvmware: 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 cmd/script uses the go:embed'ed shell sources. If you move the Go code, you need to move the shell files as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants