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

Use merge base when calculating changed files #16709

Merged
merged 16 commits into from
Aug 30, 2024

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented Aug 30, 2024

Description

get-pr-info.outputs.base.sha does not actually give the merge base, but merely the tip of the target branch. Calculate the merge base and pass it to the changed-files step.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@KyleFromNVIDIA KyleFromNVIDIA added bug Something isn't working 5 - DO NOT MERGE Hold off on merging; see PR for details non-breaking Non-breaking change labels Aug 30, 2024
@KyleFromNVIDIA KyleFromNVIDIA force-pushed the test-changed-files branch 3 times, most recently from 8c6c79a to 0143a25 Compare August 30, 2024 16:28
@KyleFromNVIDIA KyleFromNVIDIA changed the title [DO NOT MERGE] Test CI for changed-files job Use merge base when calculating changed files Aug 30, 2024
@KyleFromNVIDIA KyleFromNVIDIA added 3 - Ready for Review Ready for review by team and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Aug 30, 2024
@KyleFromNVIDIA KyleFromNVIDIA marked this pull request as draft August 30, 2024 18:24
@KyleFromNVIDIA KyleFromNVIDIA marked this pull request as ready for review August 30, 2024 19:40
@KyleFromNVIDIA
Copy link
Contributor Author

/merge

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Seems sensible enough. Presumably the error before was occurring because when the PR branch was outdated the base SHA pulled from the target branch was not actually in the history of the current branch?

@KyleFromNVIDIA
Copy link
Contributor Author

KyleFromNVIDIA commented Aug 30, 2024

Presumably the error before was occurring because when the PR branch was outdated the base SHA pulled from the target branch was not actually in the history of the current branch?

Yes, and in addition, even if it had been present, it wouldn't have correctly calculated the changed files anyway. The tj-actions/changed-files action doesn't compute the merge base on its own for push events. It does for pr events, but that doesn't help us because we're using our "copy the PR to a new branch" pattern. So the only choice is to calculate the merge base ourselves and pass it to tj-actions/changed-files.

@rapids-bot rapids-bot bot merged commit 5e420ff into rapidsai:branch-24.10 Aug 30, 2024
85 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants