knative / hack

Knative common scripts.
Apache License 2.0
18 stars 64 forks source link

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

Closed cardil closed 1 year ago

cardil commented 1 year ago

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 https://github.com/knative/hack/pull/317#issuecomment-1730266629

/kind bug

cardil commented 1 year ago

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:

https://github.com/knative/hack/blob/c4a34c34512e6aee3d8bfcc0e311fb8cd7aa2cf4/pkg/inflator/extract/extract.go#L50

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 https://github.com/knative/toolbox/pull/9)

cardil commented 1 year ago

/cc @dprotaso

cardil commented 1 year ago

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?!?

krsna-m commented 1 year ago

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 commented 1 year ago

@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: https://github.com/knative/hack/issues/318#issuecomment-1730328494

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.