opensearch-project / opensearch-build

🧰 OpenSearch / OpenSearch-Dashboards Build Systems
Apache License 2.0
141 stars 275 forks source link

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

Open prudhvigodithi opened 2 years ago

prudhvigodithi commented 2 years ago

Describe the bug

Coming from comment: https://github.com/opensearch-project/OpenSearch/pull/3481#issuecomment-1172544988 .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 commented 2 years ago

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

zelinh commented 2 years ago

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

peterzhuamazon commented 2 years ago

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 commented 2 years ago

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

peterzhuamazon commented 2 years ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 3 months ago

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

peterzhuamazon commented 3 months ago

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.