-
Notifications
You must be signed in to change notification settings - Fork 9
Add find_or_create_pull_request action
#733
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
Changes from all commits
77cdeac
5c03a93
4b39f7d
dd96bb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'fastlane/action' | ||
| require_relative '../../helper/github_helper' | ||
|
|
||
| module Fastlane | ||
| module Actions | ||
| class FindOrCreatePullRequestAction < Action | ||
| def self.run(params) | ||
| github_helper = Fastlane::Helper::GithubHelper.new(github_token: params[:github_token]) | ||
|
|
||
| existing_pr = github_helper.find_pull_request( | ||
| repository: params[:repository], | ||
| head: params[:head], | ||
| base: params[:base] | ||
| ) | ||
|
|
||
| unless existing_pr.nil? | ||
| UI.message("An open Pull Request already exists for `#{params[:head]}`: #{existing_pr.html_url}") | ||
| return existing_pr.html_url | ||
| end | ||
|
|
||
| other_action.create_pull_request( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 nit: this call supports creating drafts, we could forward a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| api_url: params[:api_url], | ||
| api_token: params[:github_token], | ||
| repo: params[:repository], | ||
| title: params[:title], | ||
| body: params[:body], | ||
| draft: params[:draft], | ||
| head: params[:head], | ||
| base: params[:base], | ||
| labels: params[:labels], | ||
| assignees: params[:assignees], | ||
| reviewers: params[:reviewers], | ||
| team_reviewers: params[:team_reviewers], | ||
| milestone: params[:milestone] | ||
| ) | ||
| end | ||
|
|
||
| def self.description | ||
| 'Returns the URL of the open Pull Request for a head branch, creating one if none exists yet' | ||
| end | ||
|
|
||
| def self.details | ||
| <<~DETAILS | ||
| Looks for an open Pull Request whose head is the given branch and which targets the given base, | ||
| and returns its URL if found. Otherwise, creates a new Pull Request and returns its URL. | ||
|
|
||
| This is useful for "rolling" automations (e.g. a daily translations or dependency-update job) that | ||
| force-push the same head branch on every run: GitHub automatically refreshes the diff of the existing | ||
| PR, so this action only needs to open a PR the first time. | ||
| DETAILS | ||
| end | ||
|
|
||
| def self.authors | ||
| ['Automattic'] | ||
| end | ||
|
|
||
| def self.return_type | ||
| :string | ||
| end | ||
|
|
||
| def self.return_value | ||
| 'The URL of the existing or newly-created Pull Request' | ||
| end | ||
|
|
||
| def self.available_options | ||
| # Parameters we forward as-is from Fastlane's `create_pull_request` action | ||
| forwarded_param_keys = %i[ | ||
| api_url | ||
| draft | ||
| labels | ||
| assignees | ||
| reviewers | ||
| team_reviewers | ||
| milestone | ||
| ].freeze | ||
|
|
||
| forwarded_params = Fastlane::Actions::CreatePullRequestAction.available_options.select do |opt| | ||
| forwarded_param_keys.include?(opt.key) | ||
| end | ||
|
|
||
| [ | ||
| *forwarded_params, | ||
| Fastlane::Helper::GithubHelper.github_token_config_item, # forwarded to `api_token` in the `create_pull_request` action | ||
| FastlaneCore::ConfigItem.new( | ||
| key: :repository, | ||
| env_name: 'GHHELPER_REPOSITORY', | ||
| description: 'The remote path of the GH repository on which we work, e.g. `wordpress-mobile/wordpress-ios`', | ||
| optional: false, | ||
| type: String | ||
| ), | ||
| FastlaneCore::ConfigItem.new( | ||
| key: :title, | ||
| description: 'The title of the Pull Request to create if none exists yet', | ||
| optional: false, | ||
| type: String | ||
| ), | ||
| FastlaneCore::ConfigItem.new( | ||
| key: :body, | ||
| description: 'The body of the Pull Request to create if none exists yet', | ||
| optional: true, | ||
| type: String | ||
| ), | ||
| FastlaneCore::ConfigItem.new( | ||
| key: :head, | ||
| description: 'The head branch of the Pull Request (the branch with the changes)', | ||
| optional: false, | ||
| type: String | ||
| ), | ||
| FastlaneCore::ConfigItem.new( | ||
| key: :base, | ||
| description: 'The base branch the Pull Request targets (e.g. `trunk`)', | ||
| optional: false, | ||
| type: String | ||
| ), | ||
| ] | ||
| end | ||
|
|
||
| def self.is_supported?(platform) | ||
| true | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'spec_helper' | ||
|
|
||
| describe Fastlane::Actions::FindOrCreatePullRequestAction do | ||
| let(:test_token) { 'ghp_fake_token' } | ||
| let(:test_repo) { 'repo-test/project-test' } | ||
| let(:test_head) { 'translations/daily-update' } | ||
| let(:test_base) { 'trunk' } | ||
| let(:github_helper) { instance_double(Fastlane::Helper::GithubHelper) } | ||
| let(:other_action_mock) { double } | ||
|
|
||
| before do | ||
| allow(Fastlane::Helper::GithubHelper).to receive(:new).with(github_token: test_token).and_return(github_helper) | ||
| allow(Fastlane::Action).to receive(:other_action).and_return(other_action_mock) | ||
| end | ||
|
|
||
| context 'when an open PR already exists for the head branch' do | ||
| it 'returns its URL and does not create a new PR' do | ||
| existing_pr = double('PullRequest', html_url: "https://github.com/#{test_repo}/pull/7") # rubocop:disable RSpec/VerifiedDoubles | ||
| allow(github_helper).to receive(:find_pull_request) | ||
| .with(repository: test_repo, head: test_head, base: test_base) | ||
| .and_return(existing_pr) | ||
| expect(other_action_mock).not_to receive(:create_pull_request) | ||
|
|
||
| result = run_described_fastlane_action( | ||
| github_token: test_token, | ||
| repository: test_repo, | ||
| title: 'Update translations', | ||
| head: test_head, | ||
| base: test_base | ||
| ) | ||
|
|
||
| expect(result).to eq("https://github.com/#{test_repo}/pull/7") | ||
| end | ||
| end | ||
|
|
||
| context 'when no open PR exists for the head branch' do | ||
| it 'creates a new PR forwarding the parameters and returns its URL' do | ||
| allow(github_helper).to receive(:find_pull_request).and_return(nil) | ||
| allow(other_action_mock).to receive(:create_pull_request).with( | ||
| api_url: 'https://api.github.com', | ||
| api_token: test_token, | ||
| repo: test_repo, | ||
| title: 'Update translations', | ||
| body: 'Sync translations from GlotPress', | ||
| draft: nil, | ||
| head: test_head, | ||
| base: test_base, | ||
| labels: ['Localization'], | ||
| assignees: nil, | ||
| reviewers: nil, | ||
| team_reviewers: nil, | ||
| milestone: nil | ||
| ).and_return("https://github.com/#{test_repo}/pull/8") | ||
|
|
||
| result = run_described_fastlane_action( | ||
| github_token: test_token, | ||
| repository: test_repo, | ||
| title: 'Update translations', | ||
| body: 'Sync translations from GlotPress', | ||
| head: test_head, | ||
| base: test_base, | ||
| labels: ['Localization'] | ||
| ) | ||
|
|
||
| expect(result).to eq("https://github.com/#{test_repo}/pull/8") | ||
| end | ||
|
|
||
| it 'forwards draft: true when creating a draft PR' do | ||
| allow(github_helper).to receive(:find_pull_request).and_return(nil) | ||
| allow(other_action_mock).to receive(:create_pull_request) | ||
| .with(hash_including(draft: true)) | ||
| .and_return("https://github.com/#{test_repo}/pull/9") | ||
|
|
||
| result = run_described_fastlane_action( | ||
| github_token: test_token, | ||
| repository: test_repo, | ||
| title: 'Update translations', | ||
| head: test_head, | ||
| base: test_base, | ||
| draft: true | ||
| ) | ||
|
|
||
| expect(result).to eq("https://github.com/#{test_repo}/pull/9") | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,46 @@ def download_file_from_tag(download_folder:) | |
| end | ||
| end | ||
|
|
||
| describe '#find_pull_request' do | ||
| let(:test_repo) { 'repo-test/project-test' } | ||
| let(:found_pr) { double('PullRequest', html_url: 'https://github.com/repo-test/project-test/pull/42') } # rubocop:disable RSpec/VerifiedDoubles | ||
| let(:client) do | ||
| instance_double( | ||
| Octokit::Client, | ||
| pull_requests: [found_pr], | ||
| user: instance_double('User', name: 'test'), | ||
| 'auto_paginate=': nil | ||
| ) | ||
| end | ||
|
|
||
| before do | ||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| end | ||
|
|
||
| it 'qualifies an unqualified head with the repository owner and forwards the base' do | ||
| expect(client).to receive(:pull_requests).with(test_repo, { state: 'open', head: 'repo-test:my-branch', base: 'trunk' }) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude: Leaving this as-is. These tests rely on the |
||
| find_pull_request(head: 'my-branch', base: 'trunk') | ||
| end | ||
|
|
||
| it 'uses an already-qualified head as-is and omits the base when not provided' do | ||
| expect(client).to receive(:pull_requests).with(test_repo, { state: 'open', head: 'someone:other-branch' }) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude: Leaving this as-is. These tests rely on the |
||
| find_pull_request(head: 'someone:other-branch') | ||
| end | ||
|
|
||
| it 'returns the first matching pull request' do | ||
| expect(find_pull_request(head: 'my-branch')).to eq(found_pr) | ||
| end | ||
|
|
||
| it 'returns nil when no pull request matches' do | ||
| allow(client).to receive(:pull_requests).and_return([]) | ||
| expect(find_pull_request(head: 'my-branch')).to be_nil | ||
| end | ||
|
|
||
| def find_pull_request(head:, base: nil) | ||
| described_class.new(github_token: 'Fake-GitHubToken-123').find_pull_request(repository: test_repo, head: head, base: base) | ||
| end | ||
| end | ||
|
|
||
| describe '#get_last_milestone' do | ||
| let(:test_repo) { 'repo-test/project-test' } | ||
| let(:last_stone) { mock_milestone('10.0') } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api_urlis used oncreate_pull_requestbut not here. I guess we'd need to change the helper for that but it probably makes sense, for consistency?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that adding
api_urlrequires changing a shared helper used across ~14 call sites, and a behavior change for create_release_backmerge_pull_request_action which will now call the custom host for@client.usercalls. That, in my opinion, requires its own PR.Since this PR follows the existing create_release_backmerge_pull_request_action from the repo, I'd prefer to leave that to a follow up if you don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, makes sense 👍