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

feat(github-actions): implement GitHub Action for creating a PR from local changes #738

Merged
merged 2 commits into from Aug 26, 2022

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Jul 24, 2022

Implement a new GitHub Action, create-pr-for-changes that can check for changes in the working copy and create a pull request. It can skip creating a pull request if there are no local changes or if a pull request already exists for the same changes and branches (based on some heuristic - see github-actions/create-pr-for-changes/lib/main.ts for details).

This action can be useful for periodically updating checked-in files that are auto-generated based on data from external sources (such as events stored in a database).

FYI, I have tested the new action with angular/angular#45588 (at commit c3e545f) with a slight modification: using the GitHub Action provided token instead of an angular-robot-key, since the Angular Robot is not installed on my fork.
Here are a couple of runs:

@gkalpak gkalpak force-pushed the feat-gh-actions-pr-for-changes branch from d73d5bb to 6368656 Compare July 24, 2022 16:54
@gkalpak gkalpak changed the title feat: implement GitHub Action for creating a PR for some changes in the working copy feat(github-actions): implement GitHub Action for creating a PR from local changes Jul 24, 2022
@gkalpak gkalpak force-pushed the feat-gh-actions-pr-for-changes branch from 6368656 to fae2dcd Compare July 24, 2022 17:10
@gkalpak gkalpak marked this pull request as ready for review July 24, 2022 17:20
// particular set of changes. It is not 100% accurate (and could result in both false
// positives and false negatives), but should be good enough for our purposes.
const branchPrefix = core.getInput('branch-prefix', {required: true});
const branchName = `${branchPrefix}-${hashFiles(touchedFiles)}`;
Copy link
Member

Choose a reason for hiding this comment

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

Why not derive the branch name using the commit sha?

${branchPrefix}-${git.run(['rev-parse', 'HEAD'])}

We could then rely on Git to create something that is even more likely to be unique.

Copy link
Member Author

@gkalpak gkalpak Aug 20, 2022

Choose a reason for hiding this comment

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

This is not just about being unique, but about uniquely identifying a set of changes (regardless of other changes that might have happened to the repo).

Imagine that we use this action to modify file foo.txt. I can have two separate SHAs that both apply the same change to file foo.txt but are different because their parent SHA is different (due to the unrelated file bar.txt being modified between the two). In such a case, we want to recognize that both commits apply the same change, even though their SHAs are different.

Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that makes sense. Will this hold for an action that doesn't always change all of the same files though? For instance if you have an action that updates all of the generated files, but some of them are unchanged.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current hashing looks at the files modified by the action run and their content. If there are two different runs that result in the same files being touched and the contents of those touched files are the same on both runs, then the two runs will be considered the same.

If you have two runs that result in a different set of files being touched (or if the contents of the touched files has changed between the two runs), then they will be considered different.

This can result in "misses" (i.e. two runs that should be identified as the same being considered different), but that is not a big problem: It just means that we might get two PRs that do the same thing.

github-actions/create-pr-for-changes/lib/main.ts Outdated Show resolved Hide resolved
github-actions/create-pr-for-changes/lib/main.ts Outdated Show resolved Hide resolved
github-actions/create-pr-for-changes/lib/main.ts Outdated Show resolved Hide resolved
github-actions/create-pr-for-changes/lib/main.ts Outdated Show resolved Hide resolved
…local changes

Implement a new GitHub Action, `create-pr-for-changes` that can check
for changes in the working copy and create a pull request. It can skip
creating a pull request if there are no local changes or if a pull
request already exists for the same changes and branches (based on some
heuristic - see `github-actions/create-pr-for-changes/lib/main.ts` for
details).

This action can be useful for periodically updating checked-in files
that are auto-generated based on data from external sources (such as
events stored in a database).
@gkalpak gkalpak force-pushed the feat-gh-actions-pr-for-changes branch 4 times, most recently from d2f9d47 to 17d093e Compare August 22, 2022 11:31
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

Just the one thing to resolve and be sure about.

@josephperrott josephperrott added action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 23, 2022
@gkalpak gkalpak removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 25, 2022
@devversion devversion merged commit eabfc94 into angular:main Aug 26, 2022
@gkalpak gkalpak deleted the feat-gh-actions-pr-for-changes branch August 26, 2022 10:12
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants