search

knative / hack

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

:bug: The `go.work.sum` don't influence the dependencies resolution #317

Closed cardil closed 1 year ago

cardil commented 1 year ago

If it's not needed. If the file isn't empty after the update, it should be committed.

Some info: https://stackoverflow.com/a/71613130

Changes

/kind bug

Follow up #316

cardil commented 1 year ago

/cc @dprotaso /cc @upodroid /cc @kvmware

cardil commented 1 year ago

What's strange, when I open the project in Goland IDE, or invoke a "Sync Go Module" operation in that IDE, the go.work.sum is being recreated with the contents of #316.

The Goland IDE must call some go command, but I don't know which one.

dprotaso commented 1 year ago

There really shouldn't be any dependencies in the hack repo - the original intention was for it to just propagate shell scripts to other repos

Now the deps used here will have an influence of the version selection (might not be a big issue) - but I would encourage moving tools to toolbox and test fixtures can be generated programmatically or put under testdata (eg. test vendor project)

dprotaso commented 1 year ago

Also note this dep is now in the stdlib

https://github.com/knative/hack/blob/c4a34c34512e6aee3d8bfcc0e311fb8cd7aa2cf4/go.mod#L6

https://pkg.go.dev/errors

cardil commented 1 year ago

@dprotaso There really shouldn't be any dependencies in the hack repo - the original intention was for it to just propagate shell scripts to other repos

Now the deps used here will have an influence of the version selection (might not be a big issue) - but I would encourage moving tools to toolbox and test fixtures can be generated programmatically or put under testdata (eg. test vendor project)

Good point. I think this is an oversight on my side, while doing the https://github.com/knative/hack/pull/222. Before that PR there ware go deps, but only in isolated go sub modules.

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

Let's capture this in a separate issue.

cardil commented 1 year ago

@dprotaso the issue https://github.com/knative/hack/issues/318

dprotaso commented 1 year ago

/lgtm /approve

cardil commented 1 year ago

/override style / suggester / shell

knative-prow[bot] commented 1 year ago

@cardil: cardil unauthorized: /override is restricted to Repo administrators.

In response to [this](https://github.com/knative/hack/pull/317#issuecomment-1730318465): >/override style / suggester / shell Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
knative-prow[bot] commented 1 year ago

@upodroid: Overrode contexts on behalf of upodroid: style / suggester / shell

In response to [this](https://github.com/knative/hack/pull/317#pullrequestreview-1639795012): >/override "style / suggester / shell" Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
knative-prow[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, dprotaso, upodroid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/knative/hack/blob/main/OWNERS)~~ [cardil,dprotaso,upodroid] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment