search

knative / test-infra

Test infrastructure for the Knative project
Apache License 2.0
82 stars 161 forks source link

Updating deps with submodules #3510

Closed cardil closed 2 years ago

cardil commented 2 years ago

Related to #3509

cardil commented 2 years ago

/assign @upodroid /assign @kvmware /cc @mgencur

knative-prow[bot] commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil

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/test-infra/blob/main/OWNERS)~~ [cardil] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
upodroid commented 2 years ago

/hold

We are landing a very large go.mod changes in #3509 next week which is required for the SLSA Generator that I'm working on.

cardil commented 2 years ago

Re @mgencur:

Sorry if this is a dumb question but this current PR should replace https://github.com/knative/test-infra/pull/3509 if I'm not mistaken. The current PR adds 2,822,424 lines and https://github.com/knative/test-infra/pull/3509 adds only 1,355,412 . Do we really need those files. Do we have a good reason to have this change?

That depends on what we want. This PR adds the "hidden" modules tools/configgen, tools/release-jobs-syncer, tools/rundk to Go workspace. This makes them work properly. Together with https://github.com/knative/hack/issues/216 it ends up vendoring all thei dependencies in appopriate vendor directories.

The #3509 is doing the oposite - it is removing the go.mod for those tools. In effect all dependencies are being vendored on root level vendor dir.

Both PRs are updating the deps, and fixing the invalid go modules structure.

The key difference between those is that mine is a bit safer, becouse the dependencies for those submodules are kept at the same version. #3509 is aliging all those version to one defined in root go.mod.

I'm fine with both approaches.

upodroid commented 2 years ago

I'm leaning towards #3509 as well (bit biased). In any case, Chris is working on removing vendor anyway so the line count doesn't really matter.

Just to note, no one should be using knative/test-infra as a library so a messy root go.mod is fine. The root go.mod is a mess because of dependency on kubernetes/test-infra and istio-test-infra (istio relies on k8s/test-infra).

If life wasn't hard enough, our k8s version is determined by knative/pkg which always lags

https://github.com/kubernetes/test-infra/blob/master/go.mod

knative-prow[bot] commented 2 years ago

@cardil: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
unit-tests_test-infra_main b181546bdffc03e0f3ca1215ef1c1fc744e9ab67 link true /test unit-tests_test-infra_main

Your PR dashboard.

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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
cardil commented 2 years ago

Lets go with https://github.com/knative/test-infra/pull/3509 approach. It's simpler.