Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: github.event.pull_request.head.sha used in Gradle check does not identify force commit sha's. #2292

Open
prudhvigodithi opened this issue Jul 1, 2022 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@prudhvigodithi
Copy link
Collaborator

prudhvigodithi commented Jul 1, 2022

Describe the bug

Coming from comment: opensearch-project/OpenSearch#3481 (comment)
.pull_request.head.sha used in gradle check does not execute on force commits.
Failed build: https://build.ci.opensearch.org/job/gradle-check/121/
Existing worflow.
https://github.com/opensearch-project/OpenSearch/blob/main/.github/workflows/gradle-check.yml#L13

To reproduce

Assuming this reproducible pushing a commit with -f and try to execute the gradle check.

Expected behavior

Handle cases even if the latest commit is done via force push.

Additional context

Some issues related to this topic:
https://github.community/t/github-actions-how-to-to-get-pr-merge-commit-sha-in-push/209044

Relevant log output

No response

@prudhvigodithi prudhvigodithi added bug Something isn't working untriaged Issues that have not yet been triaged labels Jul 1, 2022
@prudhvigodithi
Copy link
Collaborator Author

[Triaged] @peterzhuamazon can you add your thoughts ?
Thank you

@prudhvigodithi prudhvigodithi removed the untriaged Issues that have not yet been triaged label Jul 5, 2022
@zelinh
Copy link
Member

zelinh commented Jul 5, 2022

Maybe we should transfer this issue to OpenSearch repo since this is regarding of the gradle check workflow in that repo?

@peterzhuamazon
Copy link
Member

This seems like more of a problem on github side not on us.
I have no more variable to access apart from the head sha returned by github.

@bbarani
Copy link
Member

bbarani commented Jul 6, 2022

@prudhvigodithi @peterzhuamazon Is there a way to inform the user to perform a dummy commit as a workaround?

@peterzhuamazon
Copy link
Member

Since this is an issue on github side, we will close this for now.
You can simply push a new commit to re-trigger gradle check with the correct reference.
Thanks.

@prudhvigodithi
Copy link
Collaborator Author

A recent update on this matter: despite a GitHub force push, the .pull_request.head.sha still successfully retrieves the commit ID that Jenkins uses to initiate the ./gradle check process. However, if a force push occurs shortly before invoking the Jenkins gradle-check job, an error like fatal: reference is not a tree: e80ddbab5f324b449a23XXXXXXXXXXXX may arise. This happens because a force push alters the commit history of the target branch, causing it to point to a new commit.
@getsaurabh02 @peterzhuamazon

@peterzhuamazon
Copy link
Member

A recent update on this matter: despite a GitHub force push, the .pull_request.head.sha still successfully retrieves the commit ID that Jenkins uses to initiate the ./gradle check process. However, if a force push occurs shortly before invoking the Jenkins gradle-check job, an error like fatal: reference is not a tree: e80ddbab5f324b449a23XXXXXXXXXXXX may arise. This happens because a force push alters the commit history of the target branch, causing it to point to a new commit. @getsaurabh02 @peterzhuamazon

Yes @prudhvigodithi I think this is a small enough bug tho that is not that significant.
Tho there are contributors suggesting to have a better way of triggering the test or similar.
We can take a look and see. Thanks.

@prudhvigodithi
Copy link
Collaborator Author

The simple and quick fix here is before the git checkout -f ${git_reference}, we can run example git rev-parse -q --verify 26d579287f50bb33e17c8fe1f05ea208d5c64d1f > /dev/null, if this command returns 0 the commit exists and move forward and if 1 the commit does not exists we can have currentBuild.result = ABORTED

@peterzhuamazon
Copy link
Member

The simple and quick fix here is before the git checkout -f ${git_reference}, we can run example git rev-parse -q --verify 26d579287f50bb33e17c8fe1f05ea208d5c64d1f > /dev/null, if this command returns 0 the commit exists and move forward and if 1 the commit does not exists we can have currentBuild.result = ABORTED

Yes, we can start with this for now. It is easier just to abort the Jenkins workflow without let it goes to full FAILURE status.

@rishabh6788
Copy link
Collaborator

If we are allowing the user to trigger gradle-checks on force-push then we are not too concerned about a user pushing a malicious code or we already have guardrails around in.
In that case should we really checkout the head repo using sha or can we use the head branch for checking out the latest code on the contributor's pr branch and run gradle-check on that? @peterzhuamazon @prudhvigodithi

@rishabh6788 rishabh6788 reopened this Aug 12, 2024
@github-actions github-actions bot added the untriaged Issues that have not yet been triaged label Aug 12, 2024
@peterzhuamazon
Copy link
Member

If we are allowing the user to trigger gradle-checks on force-push then we are not too concerned about a user pushing a malicious code or we already have guardrails around in. In that case should we really checkout the head repo using sha or can we use the head branch for checking out the latest code on the contributor's pr branch and run gradle-check on that? @peterzhuamazon @prudhvigodithi

Using branch head have potential issues of previous trigger use next commit, due to github actions has delays. If anyone push multiple times within a short period of time, it will cause some false positives potentially.

I would still suggest cancel the github action if there is a mismatch before triggering on Jenkins.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants