add optional presubmit job to run apidiff on the client-go module

[aojea]

Having an optional presubmit that gets executed automatically when a client-go file is modified, to run apidiff against the client-go module will be helpful for reviewers to understand the impact of the changes and, if necessary, evaluate if the breaking changes are necessary or can be done in a different way that does not break downstream clients (i.e. declare the old function deprecated and add a new one)

See gist with the differences between minors versions in client-go https://gist.github.com/aojea/696155fb3e2f8775574e6afddcb33cfc , it seems we were doing better overall, but being more consistent in this area will help to reduce the friction of the ecosystem that need to consume these APIS and clients, see kubernetes/kubernetes#124380

Reference to slack discussion https://kubernetes.slack.com/archives/C0EG7JC6T/p1732746366129629

Example execution:

./hack/apidiff.sh -t 26c08dde527c0d4453e95f49e3c6ef7663042f7b -r 9d62330bfa31a4fce28093d052f65ff0e88ac3a0  ./staging/src/k8s.io/client-go
Checking 26c08dde527c0d4453e95f49e3c6ef7663042f7b (= v1.32.0-beta.0-516-g26c08dde527) for API changes since 9d62330bfa31a4fce28093d052f65ff0e88ac3a0 (= v1.33.0-alpha.0-3-g9d62330bfa3).
Installing apidiff into /tmp/tmp.yRagAHGNPP.
Preparing worktree (detached HEAD 26c08dde527)
Updating files: 100% (26242/26242), done.
HEAD is now at 26c08dde527 Wait for updated keys to be observed
Preparing worktree (detached HEAD 9d62330bfa3)
Updating files: 100% (26242/26242), done.
HEAD is now at 9d62330bfa3 Merge pull request #128286 from umagnus/fix_unmount_relative_path

## ./staging/src/k8s.io/client-go
no changes

Some notes about API differences:

Changes in internal packages are usually okay.
However, remember that custom schedulers
and scheduler plugins depend on pkg/scheduler/framework.

API changes in staging repos are more critical.
Try to avoid them as much as possible.
But sometimes changing an API is the lesser evil
and/or the impact on downstream consumers is low.
Use common sense and code searches.

[aojea]

@pohly if I run the script locally it does not work as I expect, but as we talked offline and if you see my gist it works fine, do you think is a problem of the hack script or of the environment?

./hack/apidiff.sh ./staging/src/k8s.io/client-go
Checking current working tree for API changes since 94a15929cf13354fdf3747cb266d511154f8c97b (= v1.28.0-alpha.0-315-g94a15929cf1).
Installing apidiff into /tmp/tmp.lzfNPXoYIp.
Preparing worktree (detached HEAD 94a15929cf1)
Updating files: 100% (23757/23757), done.
HEAD is now at 94a15929cf1 Merge pull request #116408 from ChenLingPeng/fit
loading ./staging/src/k8s.io/client-go: -: pattern ./staging/src/k8s.io/client-go/...: main module (k8s.io/kubernetes) does not contain package k8s.io/kubernetes/staging/src/k8s.io/client-go
!!! [1127 22:33:46] Call tree:
!!! [1127 22:33:46]  1: ./hack/apidiff.sh:184 run(...)
!!! [1127 22:33:46]  2: ./hack/apidiff.sh:200 inWorktree(...)
!!! [1127 22:33:46] Call tree:
!!! [1127 22:33:46]  1: ./hack/apidiff.sh:200 inWorktree(...)

EDIT

This seems to be a problem of getting the base from kubernetes versions without go workspaces, see description with good run

[aojea]

/assign @deads2k @liggitt @jpbetz
/cc @BenTheElder @pohly
/sig api-machinery

[liggitt]

/lgtm
/hold in case you want more eyes before merge

[aojea]

Ok, this should be ready now, but still depends on kubernetes/kubernetes#129021 to support the wildecard and the modules that are not present in the workspace for the generated code

[aojea]

/hold cancel

The k/k fixes got merged

@pohly can you PTAL?

[BenTheElder]

I recommend moving this fully out into a script somewhere, it will need iterating on and inline in prowjob config is a poor place to iterate on scripts.

I don't consider this a hard blocker, but seriously consider doing this sooner than later.
If it were planned to be a blocking job, I'd be more concerned.

The moment this apidiff script wants more computed inputs, we have a mess, but if some wrapper script takes in the prow envs, it's not a problem.

[aojea]

I don't think adding an indirection in this case is going to help, if we end needing more complexity for this job then we are doing something wrong

[BenTheElder]

usually the prowjob should make a trivial invocation like make test, and encapsulate the rest in the repo under test, where you can iterate on it with a tested PR

it's not about more complexity, it's about any change to how this needs to be invoked. right now you're passing quite a lot as arguments coming from env or even computed.

[BenTheElder]

(which also makes local replication more difficult)

[aojea]

🤔 there are only 2 arguments that depend on prow variables

[BenTheElder]

(aside: PULL_PULL_SHA will not be set on batch PR runs, TODO to rework this out if we ever want a blocking job later)

https://docs.prow.k8s.io/docs/jobs/#job-environment-variables

[pohly]

apidiff.sh was meant to be agnostic of the CI system in which it gets run. But I don't feel strongly about this.

(aside: PULL_PULL_SHA will not be set on batch PR runs, TODO to rework this out if we ever want a blocking job later)

It's complexity like this that I wanted to keep out of the script... In the job it's clearer which variables are set. 🤷

[BenTheElder]

kubernetes/kubernetes#129188

[BenTheElder]

apidiff.sh was meant to be agnostic of the CI system in which it gets run. But I don't feel strongly about this.

... eh... prow is the CI system Kubernetes uses, we normally support it in our tools / scripts.

It's complexity like this that I wanted to keep out of the script... In the job it's clearer which variables are set. 🤷

That complexity has to go somewhere, and it's harder to iterate on correctly if it's inline in the job.

[BenTheElder]

(also, we're not requiring prow-like-environment-variables, just supporting it as a fallback if the args are not otherwise set, see the PR)

[BenTheElder]

the REPO_ROOT value is wrong and should be detected in the apidiff script (or passed in via pwd if we really can't do it in the script for some reason, there's copy paste one-liners for this in k/k)

that's the only blocking concern

[BenTheElder]

this approach isn't technically guaranteed to schedule daily, you should probably use a cron for this and pick a particular time of day to run for consistency

[aojea]

it's a canary job, I just want to get some periodic signal , nothing else, if this work well we figure out a more elegant way of scheduling, but in the meantime think on it as a personal job, nothing else

[BenTheElder]

OK, it's not that complicated though ... https://docs.prow.k8s.io/docs/jobs/#how-to-configure-new-jobs

You just swap this field for cron: <normal cron string>, e.g. cron: 0 0 * * * would run every day at midnight UTC

[BenTheElder]

If the time window matches, the report will be more useful, otherwise it will vary by how many developers have recently been merging code. not blocking, but consider this in a follow-up. We can look at the merge graph in k/k.

[BenTheElder]

this thread is non-blocking, we can do this in a follow-up, but I strongly recommend it.

interval: 24h is almost always not what people actually want, interval makes sense for frequently run things where we don't want to stack up missed runs from the job potentially taking longer than the interval, and we don't care at all about timing otherwise.

pretty much any job with a long interval is using a cron instead for this reason, the scheduling is more predictable.

for a daily report you want predictable, so you can reliably check it after it's done.
the timing of interval is affected by when it was first introduced, any disruptions to prow, and any other jitter, since it's always computed since the last run.

[aojea]

the REPO_ROOT value is wrong and should be detected in the apidiff script (or passed in via pwd if we really can't do it in the script for some reason, there's copy paste one-liners for this in k/k)

that's the only blocking concern

that variable is not used, at least I didn't find where, so I removed it

[BenTheElder]

/lgtm
/approve
let's iterate on the other details.

after kubernetes/kubernetes#129188 we could gracefully drop some of the flags in presubmit and streamline it.

I'll send a follow-up to switch to a cron for the periodic.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder

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:

• config/OWNERS [BenTheElder,aojea]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[BenTheElder]

/lgtm

[BenTheElder]

/lgtm

( https://github.com/kubernetes/test-infra/compare/4321a11724bbbea693131fedab59dcbdf8f9be3d..83e564924c1f146ff5cc02b6a24dee5921e9d139 )

:-)

[k8s-ci-robot]

@aojea: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

• key apidiff.yaml using file config/jobs/kubernetes/sig-testing/apidiff.yaml

In response to this:

Having an optional presubmit that gets executed automatically when a client-go file is modified, to run apidiff against the client-go module will be helpful for reviewers to understand the impact of the changes and, if necessary, evaluate if the breaking changes are necessary or can be done in a different way that does not break downstream clients (i.e. declare the old function deprecated and add a new one)

See gist with the differences between minors versions in client-go https://gist.github.com/aojea/696155fb3e2f8775574e6afddcb33cfc , it seems we were doing better overall, but being more consistent in this area will help to reduce the friction of the ecosystem that need to consume these APIS and clients, see kubernetes/kubernetes#124380

Reference to slack discussion https://kubernetes.slack.com/archives/C0EG7JC6T/p1732746366129629

Example execution:

./hack/apidiff.sh -t 26c08dde527c0d4453e95f49e3c6ef7663042f7b -r 9d62330bfa31a4fce28093d052f65ff0e88ac3a0  ./staging/src/k8s.io/client-go
Checking 26c08dde527c0d4453e95f49e3c6ef7663042f7b (= v1.32.0-beta.0-516-g26c08dde527) for API changes since 9d62330bfa31a4fce28093d052f65ff0e88ac3a0 (= v1.33.0-alpha.0-3-g9d62330bfa3).
Installing apidiff into /tmp/tmp.yRagAHGNPP.
Preparing worktree (detached HEAD 26c08dde527)
Updating files: 100% (26242/26242), done.
HEAD is now at 26c08dde527 Wait for updated keys to be observed
Preparing worktree (detached HEAD 9d62330bfa3)
Updating files: 100% (26242/26242), done.
HEAD is now at 9d62330bfa3 Merge pull request #128286 from umagnus/fix_unmount_relative_path

## ./staging/src/k8s.io/client-go
no changes

Some notes about API differences:

Changes in internal packages are usually okay.
However, remember that custom schedulers
and scheduler plugins depend on pkg/scheduler/framework.

API changes in staging repos are more critical.
Try to avoid them as much as possible.
But sometimes changing an API is the lesser evil
and/or the impact on downstream consumers is low.
Use common sense and code searches.

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-sigs/prow repository.