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

Introduce a separate GHA workflow for Pull Request Validation to validate the correct commit hashes #269

Merged
merged 11 commits into from
Dec 17, 2024

Conversation

Will-Lo
Copy link
Collaborator

@Will-Lo Will-Lo commented Dec 11, 2024

Summary

This PR adds a new workflow which largely reuses the existing workflow for building/creating images, except that it runs on pull requests but also uses the correct hashes. The PR workflow also has some optimizations to cancel the previous test run on the same PR if a new commit is pushed since that test result is outdated.

Issue] Briefly discuss the summary of the changes made in this
pull request in 2-3 lines.

For all the boxes checked, please include additional details of the changes made in this pull request.

The workflow on the main branch appears to be using the merge commit SHA for tagging purposes: https://github.com/anothrNick/github-tag-action

However this causes issues where it can use outdated hashes of pull requests due to using github.event.pull_request.merge_commit_sha which does not seem consistent with the Pull Request HEAD SHA.

Documentation on the default behavior here that should fix it https://github.com/actions/checkout/blob/main/README.md

This can cause PRs to have breaking commits but due to this workflow file can pass the CI tests, making it difficult to work in the repository. Another side effect of the PR is that committers branches that have multiple commits may have difficulty debugging their failing workflows locally since the CI tests are running on outdated files.

See testing below for replication of the issue.

Changes

  • Client-facing API Changes
  • Internal API Changes
  • Bug Fixes
  • New Features
  • Performance Improvements
  • Code Style
  • Refactoring
  • Documentation
  • Tests

Testing Done

  • Manually Tested on local docker setup. Please include commands ran, and their output.
  • Added new tests for the changes made.
  • Updated existing tests to reflect the changes made.
  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
  • Some other form of testing like staging or soak time in production. Please explain.

For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.

Tested locally on my repo
Will-Lo#1 shows a PR with the old behavior which has a clearly breaking change, but is passing tests since the commit that opened the PR passed tests

Will-Lo#3 shows a PR that used to pass with the commit that opened it, but now fails with the same breaking change.

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

@Will-Lo Will-Lo marked this pull request as ready for review December 11, 2024 04:56
@Will-Lo Will-Lo changed the title Change github actions to use the default checkout sha, the sha of the… Change Github Actions Workflow to use the SHA of the commit that started the workflow Dec 11, 2024
@Will-Lo Will-Lo changed the title Change Github Actions Workflow to use the SHA of the commit that started the workflow Introduce a separate GHA workflow for Pull Request Validation to validate the correct commit hashes Dec 11, 2024
Copy link
Collaborator

@autumnust autumnust left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this problem !
I may need some education on how this thing work after the change. left a comment

.github/workflows/build-test-pr.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@dxichen dxichen left a comment

Choose a reason for hiding this comment

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

Cleanup looks great, thanks!

Copy link
Collaborator

@autumnust autumnust left a comment

Choose a reason for hiding this comment

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

Nice segment-reuse in build-run-tests !

@Will-Lo Will-Lo merged commit eaf36db into linkedin:main Dec 17, 2024
1 check passed
@Will-Lo Will-Lo deleted the fix-github-actions-commit-sha branch December 17, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants