Skip to content

Inject a GetFilesChanged shim instead of using GithubService directly. #4178

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 7 commits into from
Jan 25, 2025
Merged

Inject a GetFilesChanged shim instead of using GithubService directly. #4178

auto-submit merged 7 commits into from
Jan 25, 2025

Conversation

@matanlurey
Copy link
Contributor

@matanlurey matanlurey commented Jan 25, 2025

Towards flutter/flutter#161462, towards flutter/flutter#162201.

This PR does a few things:

  1. Introduces an abstraction, GetFilesChanged
  2. Codifies "we aren't confident" as InconclusiveFilesChanged
  3. Refactors the existing implementation into GithubApiGetFilesChanged

The intention is no user-facing behavioral changes, but this PR enables:

  1. Swapping out the implementation (i.e. to CheckRunArtifactsGetFilesChanged)
  2. Reusing the logic, including inconclusive-ness, for Conditionally skip the engine build phase flutter#162201 (framework-only PRs).

jtmcdole
jtmcdole reviewed Jan 25, 2025
View reviewed changes
app_dart/lib/src/foundation/utils.dart Outdated
switch (filesChanged) {
case InconclusiveFilesChanged(:final pullRequestNumber, :final reason):
log.info('Running all targets on PR#$pullRequestNumber: $reason');
return targets.toList();
Copy link
Member

@jtmcdole jtmcdole Jan 25, 2025

Choose a reason for hiding this comment

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

[...targets]

@matanlurey
Copy link
Contributor Author

matanlurey commented Jan 25, 2025

@yjbanov I am going to submit this to unblock Part 2 Monday, but if you have any feedback or suggestions leave them and I'll send a follow-up PR. Thanks again!

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App. label Jan 25, 2025
@auto-submit auto-submit bot merged commit 86f29e5 into flutter:main Jan 25, 2025
4 checks passed
@matanlurey matanlurey mentioned this pull request Jan 25, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtmcdole jtmcdole jtmcdole approved these changes

@yjbanov yjbanov Awaiting requested review from yjbanov

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@matanlurey @jtmcdole