Apidiff test runs only if the changes are in api/ and exp/api/

[meghanajangi]

What type of PR is this?
/kind failing-test

What this PR does / why we need it:
This PR will limit the apidiff test to run only if there are changes made in the directory api/ and exp/api/

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2051

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

NONE

[mboersma]

Thanks @meghanajangi! I've tested the essential command here git --no-pager diff --name-only FETCH_HEAD | grep "api/" locally and read up on FETCH_HEAD. This seems like it should do the right thing.

But looking more closely, I see that the pull-cluster-api-provider-azure-apidiff job ran as usual. Since this PR itself didn't change anything under api/ or exp/api/, it should have been a no-op.

The Makefile actually isn't used in this case. Instead, ./scripts/ci-apidiff.sh is called directly by the job defined here in kubernetes/test-infra.

If you can make a similar change to the ci-apidiff.sh script instead, it should work in all cases. And maybe it should print out "No files under api/ or exp/api/ changed" or "nothing to do" in that case.

[mboersma]

Another approach that might be preferable in this case is to configure the presubmit prow job with a run_if_changed regular expression. Then it won't even show up in the list of GitHub checks below unless it was needed.

Also see testing prow jobs locally.

[meghanajangi]

Another approach that might be preferable in this case is to configure the presubmit prow job with a run_if_changed regular expression. Then it won't even show up in the list of GitHub checks below unless it was needed.

Also see testing prow jobs locally.

Thank you for the comments @mboersma . I made the changes to ci-apidiff script as you suggested in the first comment.
I also made a change in the script and Makefile similar to this CAPI PR to deduplicate apidiff logic.

[mboersma]

/retest

Unrelated provisioning flake.

[mboersma]

/lgtm

[CecileRobertMichon]

/approve

thank you!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 [CecileRobertMichon]

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

[jackfrancis]

I think if we grep'd for "/api/" (both forward and trailing slashes) we'd be a bit more defensive (in a good way) going forward. For example, if someone created a subdirectory somewhere with a substring suffix of "capi" this would match, which we wouldn't want.

Thanks for doing this!

[mboersma]

I don't think this change with the leading slash will work correctly, though:

% git --no-pager diff --name-only FETCH_HEAD | grep "/api/"
% echo dummy >> api/v1beta1/doc.go                         
% git --no-pager diff --name-only FETCH_HEAD | grep "/api/"
% git --no-pager diff --name-only FETCH_HEAD | grep "api/" 
api/v1beta1/doc.go

[CecileRobertMichon]

Good point @mboersma. I think grep "^api/\|/api/" is what we want (it will match both the api/ dir and the exp/api/ dir)

[CecileRobertMichon]

/hold
/lgtm cancel

to address @jackfrancis' comment above

[meghanajangi]

@jackfrancis @CecileRobertMichon I have made the requested change. PTAL 🙃

[meghanajangi]

/retest

[mboersma]

/lgtm

This should work, as far as I can tell in local testing.

Thanks for your patience @meghanajangi!

[jackfrancis]

lgtm

[jackfrancis]

@mboersma after this merges could you create a draft PR that validates this? (I'd do it myself, but I'm not familiar enough with how to trip up apidiff using present config)

[mboersma]

could you create a draft PR that validates this?

Good call–sure, I'll do that.

[mboersma]

/hold cancel