Infra - Fix API compatibility Github Action

[kbendick]

The API Binary compatibility task revapi, was previously skipping or occasionally passing when it shouldn't be (and when it was not passing for local gradle runs).

The issues were two:

1. The cache action was causing the action to be skipped.
2. The git checkout action checks out by default at a depth of 1, which is a detached head state. Therefore, the output of git describe or the first line of the output of git tags was not showing up. The action uses this tag to find the proper existing overrides and "old" version to pull from maven (currently iceberg-api:0.13.0)

This removes the cache action, adds a step to output the git describe (so we can easily verify which tag is considered current, which corresponds to what's in .palantir/revapi.yml), and then runs the revapi task pretty aggressively (with --rerun-tasks, --no-build-cache, and --no-daemon).

We can try adding back in the actions/cache action as well as removing some of the aggressive flags for iceberg-api:revapi, but for now this is what I tested with in my fork and this will work. 🙂

[kbendick]

This PR replaces #4974

[kbendick]

cc @singhpk234 @ajantha-bhat @findepi

I tested this exact configuration in my fork and it worked as seen here: https://github.com/kbendick/iceberg/runs/6780458052?check_suite_focus=true

Please see the PR summary for my explanation of the issue.

We can likely slowly add back in some of the cache layers, as I think the primary issue was the fact that it was checked out in a detached HEAD state (as shown by the log output of checkout action and as discussed in this issue.

However for now I would like to merge in something that I have seen work for sure and then we can make this more efficient.

[kbendick]

I just checked in my fork and we can remove these flags --rerun-tasks, --no-build-cache, and --no-daemon. Though --no-daemon might be desirable to some degree.

I'm going to leave --rerun-tasks in, in case we add the actions/cache back in. Though this only builds api when running and is pretty fast still.

[kbendick]

We can potentially add back in the cache step, but for now I'd like to get this working in master before fiddling with the individual arguments any further.

[singhpk234]

Thanks @kbendick for the change and comprehensive deep-dive, the above approach LGTM !!

[question] : Have a small question, based on this article

that a pull_request workflow ref would look like refs/remotes/pull/##/merge, SHA of a pull_request workflow doesn't matches the commit that triggered the workflow; instead the SHA of the pull_request is the resulting commit that was created from merging the base to the head.

Are we ok failing in the CI incase there is a merge conflict when merging base to the head ? Thoughts.
(I think we should be ok, considering we need to resolve them any how before final merge.)

[kbendick]

Thanks @kbendick for the change and comprehensive deep-dive, the above approach LGTM !!

[question] : Have a small question, based on this article

that a pull_request workflow ref would look like refs/remotes/pull/##/merge, SHA of a pull_request workflow doesn't matches the commit that triggered the workflow; instead the SHA of the pull_request is the resulting commit that was created from merging the base to the head.

Are we ok failing in the CI incase there is a merge conflict when merging base to the head ? Thoughts. (I think we should be ok, considering we need to resolve them any how before final merge.)

Adding a specific ref has been removed @singhpk234. So in the case of a merge request, we do want it to grab the branch that is the default for the action. We don't want to grab the SHA of just the commit before, but the whole branch (ie the result of squashing it).

But I agree, we should be ok as we need to resolve them anyhow before final merge. And I'd rather it give a false positive (a failure that is not an actual failure) than false negatives. If we do encounter a false positive, that will trigger somebody to go look and follow up.

And like you said, we have to deal with this before merging anyway.

[kbendick]

This is just to match our other actions.

[kbendick]

This should be good to go now once tests pass @findepi @singhpk234

[kbendick]

Thanks @kbendick for the change and comprehensive deep-dive, the above approach LGTM !!

[question] : Have a small question, based on this article

that a pull_request workflow ref would look like refs/remotes/pull/##/merge, SHA of a pull_request workflow doesn't matches the commit that triggered the workflow; instead the SHA of the pull_request is the resulting commit that was created from merging the base to the head.

Are we ok failing in the CI incase there is a merge conflict when merging base to the head ? Thoughts. (I think we should be ok, considering we need to resolve them any how before final merge.)

So if I understand the article correctly, we _might _ still need to use the specific branch for pull_request.

I tested in my fork without it, and it was fine.

Additionally, the article states that in the worse case we'll get false positives. I'd rather that than false negatives as mentioned.

Let's go with this simpler workflow for now as I've tested it extensively in my fork and if we get false positives - then we'll catch them and deal with it then. But thanks for the article link, it's very helpful. Same applies for false negatives which we should catch eventually as people run via the CLI which should always generate the proper result.

[rdblue]

Thanks, @kbendick!