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

HDDS-4017. Acceptance check may run against wrong commit #1249

Merged
merged 1 commit into from Jul 27, 2020

Conversation

adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Jul 23, 2020

What changes were proposed in this pull request?

Use commit SHA to checkout a specific commit on push and schedule events. Keep using name for pull_request events (eg. refs/pull/1234/merge), because merge commit may not be available by SHA.

Use github context instead of environment variables. Github substitutes these values before running the command, so we can see them in the log. Comparison:

git clone https://github.com/${GITHUB_REPOSITORY}.git /mnt/ozone
cd /mnt/ozone
git fetch origin "${GITHUB_REF}"
git checkout FETCH_HEAD

vs.

git clone https://github.com/adoroszlai/hadoop-ozone.git /mnt/ozone
cd /mnt/ozone
git fetch origin "refs/heads/HDDS-4017"
git checkout FETCH_HEAD

https://issues.apache.org/jira/browse/HDDS-4017

How was this patch tested?

push event:
https://github.com/adoroszlai/hadoop-ozone/runs/903007668#step:4:8

pull_request event:
https://github.com/apache/hadoop-ozone/pull/1249/checks?check_run_id=903076398#step:4:6

@adoroszlai adoroszlai self-assigned this Jul 23, 2020
@adoroszlai adoroszlai requested a review from elek July 24, 2020 09:00
Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks the patch @adoroszlai

I am fine with it, but have a question (to improve my understanding):

Based on my understanding for pull requests the merge commit points to a potential merge. In case of using github checkout action, not the github.ref but the merge commit is used.

Do you have any example when it was wrong?

PS:

FYI: I started to experiment with a custom checkout script:

elek#13

My main motivation was to always merge to the latest master with coverage data to have better codecov data.

@adoroszlai
Copy link
Contributor Author

Based on my understanding for pull requests the merge commit points to a potential merge. In case of using github checkout action, not the github.ref but the merge commit is used.

Both ${GITHUB_REF} and ${{ github.ref }} point to the merge commit for PRs, and that's fine.

The problem is with push builds, because in that case both refs are something like refs/heads/master. The commit this points to can change in the meantime, so we need to use the SHA. (Your original solution was fine for push commits, my fix for PRs introduced this problem.)

Do you have any example when it was wrong?

The Jira issue HDDS-4017 has an example.

@elek
Copy link
Member

elek commented Jul 27, 2020

The commit this points to can change in the meantime, so we need to use the SHA.

Thanks to explain it. Now I got it. 💡 💡 💡

Let me merge it.

@elek elek merged commit fd47f91 into apache:master Jul 27, 2020
@adoroszlai adoroszlai deleted the HDDS-4017 branch July 27, 2020 15:36
@adoroszlai
Copy link
Contributor Author

Thanks @elek for reviewing and committing it.

errose28 pushed a commit to errose28/ozone that referenced this pull request Jul 31, 2020
* master: (55 commits)
  HDDS-4052. Remove master/slave terminology from Ozone (apache#1281)
  HDDS-4047. OzoneManager met NPE exception while getServiceList (apache#1277)
  HDDS-3990. Test Kubernetes examples with acceptance tests (apache#1223)
  HDDS-4045. Add more ignore rules to the RAT ignore list (apache#1273)
  HDDS-3970. Enabling TestStorageContainerManager with all failures addressed (apache#1257)
  HDDS-4033. Make the acceptance test reports hierarchical (apache#1263)
  HDDS-3423. Enabling TestContainerReplicationEndToEnd and addressing failures (apache#1260)
  HDDS-4027. Suppress ERROR message when SCM attempt to create additional pipelines. (apache#1265)
  HDDS-4024. Avoid while loop too soon when exception happen (apache#1253)
  HDDS-3809. Make number of open containers on a datanode a function of no of volumes reported by it. (apache#1081)
  HDDS-4019. Show the storageDir while need init om or scm (apache#1248)
  HDDS-3511. Fix javadoc comment in OmMetadataManager (apache#1247)
  HDDS-4041. Ozone /conf endpoint triggers kerberos replay error when SPNEGO is enabled. (apache#1267)
  HDDS-4031. Run shell tests in CI (apache#1261)
  HDDS-4038. Eliminate GitHub check warnings (apache#1268)
  HDDS-4011. Update S3 related documentation. (apache#1245)
  HDDS-4030. Remember the selected columns and make the X-axis scrollable in recon datanodes UI (apache#1259)
  HDDS-4032. Run author check without docker (apache#1262)
  HDDS-4026. Dir rename failed when sets 'ozone.om.enable.filesystem.paths' to true (apache#1256)
  HDDS-4017. Acceptance check may run against wrong commit (apache#1249)
  ...
rakeshadr pushed a commit to rakeshadr/hadoop-ozone that referenced this pull request Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants