You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/07 19:00:35 UTC
[GitHub] [iceberg] kbendick opened a new pull request, #4989: Infra - Fix API compatibility Github Action
kbendick opened a new pull request, #4989:
URL: https://github.com/apache/iceberg/pull/4989
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. 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#discussion_r891633026
##########
.github/workflows/api-binary-compatibility.yml:
##########
@@ -38,15 +38,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
+ if: github.event_name == 'pull_request'
+ with:
+ # fetch-depth of zero ensures that the tags are pulled in and we're not in a detached HEAD state
+ # revapi depends on the tags, specifically the tag from git describe, to find the relevant override
+ # in the .palantir/revapi.yml file
+ #
+ # See https://github.com/actions/checkout/issues/124
+ fetch-depth: 0
+ ref: ${{ github.event.pull_request.head.ref }}
+ - uses: actions/checkout@v2
+ if: github.event_name == 'push'
+ with:
+ fetch-depth: 0
- uses: actions/setup-java@v1
with:
java-version: 11
- - uses: actions/cache@v2
- with:
- path: ~/.gradle/caches
- key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }}
- restore-keys: ${{ runner.os }}-gradle
- - run: ./gradlew :iceberg-api:revapi
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1150523491
Thanks, @kbendick!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] findepi commented on a diff in pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#discussion_r892015058
##########
.github/workflows/api-binary-compatibility.yml:
##########
@@ -38,15 +38,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
+ if: github.event_name == 'pull_request'
+ with:
+ # fetch-depth of zero ensures that the tags are pulled in and we're not in a detached HEAD state
+ # revapi depends on the tags, specifically the tag from git describe, to find the relevant override
+ # in the .palantir/revapi.yml file
+ #
+ # See https://github.com/actions/checkout/issues/124
+ fetch-depth: 0
+ ref: ${{ github.event.pull_request.head.ref }}
Review Comment:
the point is -- i don't know why you want to specify `ref` at all, instead of relying on checkout action's default behavior.
if you did go with defaults, you wouldn't need these `if`s, so it would be simpler, so there must be a reason not to.
IMO the reason should be documented as a code-level comment (unless it's obvious, which doesn't seem to be the case)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#discussion_r892708718
##########
.github/workflows/api-binary-compatibility.yml:
##########
@@ -38,15 +38,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
+ if: github.event_name == 'pull_request'
+ with:
+ # fetch-depth of zero ensures that the tags are pulled in and we're not in a detached HEAD state
+ # revapi depends on the tags, specifically the tag from git describe, to find the relevant override
+ # in the .palantir/revapi.yml file
+ #
+ # See https://github.com/actions/checkout/issues/124
+ fetch-depth: 0
+ ref: ${{ github.event.pull_request.head.ref }}
Review Comment:
I pulled this from the most voted answer in the issue - which is hard to find because it's got some links but several people confirmed it. And then I checked it as well in my fork.
But I think you're right. It's hard to track down the actions@v2 docs and I didn't want to be the first to upgrade, but I forgot that we already have upgraded other actions to `checkout@v3`. It's a little harder for me to test given I don't have the full permissions in this repository (or possibly just don't know the correct workflow).
Let me try what you suggested in my fork really quickly as I do believe you are right looking closer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] findepi commented on a diff in pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#discussion_r891632668
##########
.github/workflows/api-binary-compatibility.yml:
##########
@@ -38,15 +38,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
+ if: github.event_name == 'pull_request'
+ with:
+ # fetch-depth of zero ensures that the tags are pulled in and we're not in a detached HEAD state
+ # revapi depends on the tags, specifically the tag from git describe, to find the relevant override
+ # in the .palantir/revapi.yml file
+ #
+ # See https://github.com/actions/checkout/issues/124
+ fetch-depth: 0
+ ref: ${{ github.event.pull_request.head.ref }}
Review Comment:
will this run on a merge commit between target branch and the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1149072306
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1149052738
This PR replaces https://github.com/apache/iceberg/pull/4974
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1149058249
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](https://github.com/actions/checkout/issues/124).
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#discussion_r892719420
##########
.github/workflows/api-binary-compatibility.yml:
##########
@@ -38,15 +38,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
+ if: github.event_name == 'pull_request'
+ with:
+ # fetch-depth of zero ensures that the tags are pulled in and we're not in a detached HEAD state
+ # revapi depends on the tags, specifically the tag from git describe, to find the relevant override
+ # in the .palantir/revapi.yml file
+ #
+ # See https://github.com/actions/checkout/issues/124
+ fetch-depth: 0
+ ref: ${{ github.event.pull_request.head.ref }}
Review Comment:
Ok I tested this in my fork and it seems fine. Removed the complex logic @findepi.
I realized the additional logic was for people who need to commit within the action.
Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#discussion_r892720229
##########
.github/workflows/api-binary-compatibility.yml:
##########
@@ -35,18 +35,22 @@ concurrency:
jobs:
revapi:
- runs-on: ubuntu-latest
+ runs-on: ubuntu-20.04
Review Comment:
This is just to match our other actions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4989:
URL: https://github.com/apache/iceberg/pull/4989
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#discussion_r891834499
##########
.github/workflows/api-binary-compatibility.yml:
##########
@@ -38,15 +38,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
+ if: github.event_name == 'pull_request'
+ with:
+ # fetch-depth of zero ensures that the tags are pulled in and we're not in a detached HEAD state
+ # revapi depends on the tags, specifically the tag from git describe, to find the relevant override
+ # in the .palantir/revapi.yml file
+ #
+ # See https://github.com/actions/checkout/issues/124
+ fetch-depth: 0
+ ref: ${{ github.event.pull_request.head.ref }}
Review Comment:
Yes, I believe it should. Somewhat hard to test admittedly. But the merge commit counts as a `push` against the target branch. For us, that’s typically master so keeping the default is fine.
I got this from the docs of the action and the associated issue. That’s why there’s a checkout if push step just below.
We can double check once it is merged in though. For our current uses (merging into master), it’ll be fine. I’ll double check whether the GitHub context has anything on push that we might grab as well though for the ref field.
Good question @findepi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1150244905
This should be good to go now once tests pass @findepi @singhpk234
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1149497348
Thanks @kbendick for the change and comprehensive deep-dive, the above approach LGTM !!
[question] : based on this [article](https://frontside.com/blog/2020-05-26-github-actions-pull_request/#how-does-pull_request-affect-actionscheckout)
> 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.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1150243728
> Thanks @kbendick for the change and comprehensive deep-dive, the above approach LGTM !!
>
> [question] : Have a small question, based on this [article](https://frontside.com/blog/2020-05-26-github-actions-pull_request/#how-does-pull_request-affect-actionscheckout)
>
> > 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.)
This has been removed @singhpk234. So we're using the default checkout branch, but just depth of 0 instead of 1. =)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4989: Infra - Fix API compatibility Github Action
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4989:
URL: https://github.com/apache/iceberg/pull/4989#issuecomment-1150262358
> Thanks @kbendick for the change and comprehensive deep-dive, the above approach LGTM !!
>
> [question] : Have a small question, based on this [article](https://frontside.com/blog/2020-05-26-github-actions-pull_request/#how-does-pull_request-affect-actionscheckout)
>
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org