test-infra icon indicating copy to clipboard operation
test-infra copied to clipboard

don't use generics yet

Open BenTheElder opened this issue 3 years ago • 13 comments

Using generics in this repo forces all dependent projects to switch to go 1.18 + generics.

Notably, boskos depends on prow utility packages, and then projects like kubetest2 use boskos's client library ... https://github.com/kubernetes-sigs/kubetest2/pull/207

This is not particularly tenable when we have to run in environments with Kubernetes release branches that use older go and won't be upgrading or using genericgs, see also: https://github.com/kubernetes/community/pull/6693

The best alternative I see is ensuring nothing else depends on test-infra, I'm not sure if that only means boskos / boskos client users at the moment.

xref: https://github.com/kubernetes/test-infra/pull/25917 cc @alvaroaleman @chaodaiG

BenTheElder avatar Aug 01 '22 17:08 BenTheElder

/sig testing

palnabarun avatar Aug 01 '22 17:08 palnabarun

So far the assumption has been that this repo is fairly self-contained and that if we want to upgrade e.G. the go version, we can just do that. I wasn't aware that this changed.

This might be visible right now with generics, but the same could happen for any new feature or stdlib addition we use from a newer go version. If there is any expectation on how fast we can or can not update go or any other dependency, we should IMHO be very explicit about that.

Maybe we can make the boskos client lib a submodule of boskos? Ideally, that thing should only depend on the boskos api and the go stdlib for the http client.

alvaroaleman avatar Aug 02 '22 14:08 alvaroaleman

This might be visible right now with generics, but the same could happen for any new feature or stdlib addition we use from a newer go version. If there is any expectation on how fast we can or can not update go or any other dependency, we should IMHO be very explicit about that.

yeah, unfortunately this is also hard to manage in go, while the go.mod go statement gates language features like generics, there's nothing like this for the stdlib.

Maybe we can make the boskos client lib a submodule of boskos? Ideally, that thing should only depend on the boskos api and the go stdlib for the http client.

you would think, but actually that's the issue here, having generic code in test-infra is one thing, but boskos client depends on the prow secret agent, so that breaks compilation https://github.com/kubernetes-sigs/boskos/blob/35595e83c0806a9f0f86498d28eb5a25ef30897d/client/client.go#L41

BenTheElder avatar Aug 02 '22 21:08 BenTheElder

So far the assumption has been that this repo is fairly self-contained and that if we want to upgrade e.G. the go version, we can just do that. I wasn't aware that this changed.

It hasn't been since boskos was broken out, in particular. Actually until last week we had a dependency cycle between the repos 🔥 https://github.com/kubernetes/test-infra/pull/26956

BenTheElder avatar Aug 02 '22 21:08 BenTheElder

So also, even if we broke the boskos client out into it's own package, it's actually the boskos client causing issues already (AFAIK go only tries to compile packages that are actually imported into a binary ...), due to use of the secret agent

Otherwise i'd just fork the boskos client into kubetest2 for now like we merged with kubetest in test-infra and punt on the rest.

BenTheElder avatar Aug 04 '22 02:08 BenTheElder

If this is just about kubetest, what keeps us from just upgrading its go version?

alvaroaleman avatar Aug 04 '22 03:08 alvaroaleman

It’s “go install”ed by developers and CI, and when on older currently supported releases of kubernetes the toolchains required lack it.

switching everyone to prebuilt binaries or something is impractical and this hasn’t been an issue for O(years), probably because of the previous poor state of toolchains in test-infra …

BenTheElder avatar Aug 04 '22 04:08 BenTheElder

We really should be deciding: is test-infra supported as a library? If so, we should still consider back pedaling forcing generics yet IMO, the with parser api could be reworked to be Unmarshal style. If not we should undo boskos heavily importing test-infra packages, which we previously rejected.

BenTheElder avatar Aug 04 '22 04:08 BenTheElder

I believe our statement to is test-infra supported as a library is no. Openshift has a repo that heavily depends on test-infra so we informally agreed on asking before removing functionality. Other than that though I think we always assumed that it is self-contained and has no external consumers. We at some point had an issue open to clarify the boundaries of that but it never got traction: https://github.com/kubernetes/test-infra/issues/21268

For this specific occurence I think it doesn't actually need generics, I can look into that maybe later this week. We should still be clear about what our policy for the general case is though and if we act like we are a lib. cc @chaodaiG @cjwagner

alvaroaleman avatar Aug 08 '22 22:08 alvaroaleman

We should still be clear about what our policy for the general case is though and if we act like we are a lib. cc @chaodaiG @cjwagner

In my opinion, we should not act like a library in our current state. We don't identify or control what is exposed so it isn't practical for contributors to anticipate arbitrary external consumption. Continually avoiding changing any previously created public function would make refactoring difficult and grow our tech debt.

cjwagner avatar Aug 09 '22 20:08 cjwagner

ACK, I think we should probably note this somewhere, and boskos probably needs to move off of a number of prow util packages.

https://github.com/kubernetes-sigs/boskos/search?q=k8s.io%2Ftest-infra shows at least:

prow/config
prow/config/secret
prow/flagutil
prow/interrupts
prow/logrusutil
prow/metrics
prow/pjutil
prow/pjutil/pprof
prow/simplifypath

What's an acceptable path forward for that? Fork them into boskos? Create an actual library repo?

In the short term, I think we can unblock upgrading boskos client users just by eliminating the dependency on prow/config/secret.Agent in boskos/client

BenTheElder avatar Aug 09 '22 21:08 BenTheElder

I think creating an actual library repo is a better solution in the long run. For now, we can start with forking secret.Agent which doesn't use Generics to boskos. This will let consumers of boskos to use the latest boskos version.

With time, all the util like packages can move to a library repo like prow-utils or test-infra-utils.

palnabarun avatar Aug 10 '22 12:08 palnabarun

I think creating an actual library repo is a better solution in the long run

probably? but it requires buy-in from the folks actually developing this code.

For now, we can start with forking secret.Agent which doesn't use Generics to boskos. This will let consumers of boskos to use the latest boskos version.

Yeah, I think this is the most reasonable step, boskos users should mostly be either using binaries or the client library, and the client library only imports prow/config/secret so we can cut the test-infra dependency for the client library at least and mitigate most of the issue.

However, if test-infra is not intended to be used as a library, we still have a problem with boskos using it heavily elsewhere.

BenTheElder avatar Aug 11 '22 17:08 BenTheElder

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Nov 09 '22 18:11 k8s-triage-robot

/lifecycle frozen

jihoon-seo avatar Dec 01 '22 09:12 jihoon-seo

been a while :) i think we are ok now to use generics

/close

dims avatar Feb 10 '24 02:02 dims

@dims: Closing this issue.

In response to this:

been a while :) i think we are ok now to use generics

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar Feb 10 '24 02:02 k8s-ci-robot