From a48e9afb84a954d18a00bfc3c0ff6239fbacc8d0 Mon Sep 17 00:00:00 2001 From: Alex Tomlins Date: Tue, 22 Nov 2016 14:56:55 +0000 Subject: [PATCH 1/3] Use keyword args for Repository#pull_requests. This makes it consistent with the rest of the methods in this class, and makes it easier to add additional arguments. --- assets/lib/repository.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/assets/lib/repository.rb b/assets/lib/repository.rb index 0b37503..7a4c71a 100644 --- a/assets/lib/repository.rb +++ b/assets/lib/repository.rb @@ -8,8 +8,8 @@ def initialize(name:) @name = name end - def pull_requests(args = {}) - @pull_requests ||= Octokit.pulls(name, pulls_options(args)).map do |pr| + def pull_requests(base: nil) + @pull_requests ||= Octokit.pulls(name, pulls_options(base: base)).map do |pr| PullRequest.new(repo: self, pr: pr) end end From 9fb184e60c31836a6976dd60c191c500afac2639 Mon Sep 17 00:00:00 2001 From: henrytk Date: Tue, 22 Nov 2016 09:25:01 +0000 Subject: [PATCH 2/3] Add ability to disallow pull requests from forks In some cases you may want to only allow pull requests from within your own team. This is useful in scenarios where you cannot have untrusted code being pulled into your Concourse pipeline. This commit adds the ability to optionally set a `disallow_forks` configuration, which if set to true will only consider pull requests valid if they are raised from within the same repository. The value of this configuration is false by default and does not interfere with the normal behaviour of the resource. --- README.md | 2 ++ assets/lib/commands/check.rb | 13 +++++++++++-- assets/lib/pull_request.rb | 12 ++++++++++++ assets/lib/repository.rb | 24 ++++++++++++++++-------- spec/commands/check_spec.rb | 33 ++++++++++++++++++++++++++++----- 5 files changed, 69 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index c26fc26..6a2474c 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,8 @@ concourse version 1.2.x and higher and the [`version: every`](http://concourse.c See the [`git-config(1)` manual page](https://www.kernel.org/pub/software/scm/git/docs/git-config.html) for more information and documentation of existing git options. +* `disallow_forks`: *Optional*. If set to `true` it will only detect pull requests raised from within the same repository. Pull requests from forks will not generate new versions of the resource within Concourse. The default value is `false`. + ## Behavior ### `check`: Check for new pull requests diff --git a/assets/lib/commands/check.rb b/assets/lib/commands/check.rb index 166ce57..bc186ef 100755 --- a/assets/lib/commands/check.rb +++ b/assets/lib/commands/check.rb @@ -10,7 +10,7 @@ class Check < Commands::Base def output if return_all_versions? - repo.pull_requests + all_pull_requests else next_pull_request end @@ -22,11 +22,20 @@ def return_all_versions? input['source']['every'] == true end + def disallow_forks? + input['source']['disallow_forks'] == true + end + + def all_pull_requests + repo.pull_requests(disallow_forks: disallow_forks?) + end + def next_pull_request pull_request = repo.next_pull_request( id: input['version']['pr'], sha: input['version']['ref'], - base: input['source']['base'] + base: input['source']['base'], + disallow_forks: disallow_forks?, ) if pull_request diff --git a/assets/lib/pull_request.rb b/assets/lib/pull_request.rb index 2a4645d..de0f6c4 100644 --- a/assets/lib/pull_request.rb +++ b/assets/lib/pull_request.rb @@ -10,6 +10,10 @@ def ready? statuses.empty? end + def from_fork? + base_repo != head_repo + end + def equals?(id:, sha:) [self.sha, self.id.to_s] == [sha, id.to_s] end @@ -36,6 +40,14 @@ def url private + def base_repo + @pr['base']['repo']['full_name'] + end + + def head_repo + @pr['head']['repo']['full_name'] + end + def statuses @statuses ||= Octokit.statuses(@repo.name, sha).select do |status| status['context'] =~ /^concourse-ci/ diff --git a/assets/lib/repository.rb b/assets/lib/repository.rb index 7a4c71a..11d8946 100644 --- a/assets/lib/repository.rb +++ b/assets/lib/repository.rb @@ -8,10 +8,8 @@ def initialize(name:) @name = name end - def pull_requests(base: nil) - @pull_requests ||= Octokit.pulls(name, pulls_options(base: base)).map do |pr| - PullRequest.new(repo: self, pr: pr) - end + def pull_requests(base: nil, disallow_forks: false) + @pull_requests ||= load_pull_requests(base: base, disallow_forks: disallow_forks) end def pull_request(id:) @@ -19,21 +17,31 @@ def pull_request(id:) PullRequest.new(repo: self, pr: pr) end - def next_pull_request(id: nil, sha: nil, base: nil) - return if pull_requests(base: base).empty? + def next_pull_request(id: nil, sha: nil, base: nil, disallow_forks: false) + return if pull_requests(base: base, disallow_forks: disallow_forks).empty? if id && sha - current = pull_requests.find { |pr| pr.equals?(id: id, sha: sha) } + current = pull_requests(disallow_forks: disallow_forks).find { |pr| pr.equals?(id: id, sha: sha) } return if current && current.ready? end - pull_requests.find do |pr| + pull_requests(disallow_forks: disallow_forks).find do |pr| pr != current && pr.ready? end end private + def load_pull_requests(base: nil, disallow_forks: false) + pulls = Octokit.pulls(name, pulls_options(base: base)).map do |pr| + PullRequest.new(repo: self, pr: pr) + end + if disallow_forks + pulls = pulls.select { |pr| !pr.from_fork? } + end + pulls + end + def pulls_options(base: nil) base ? default_opts.merge(base: base) : default_opts end diff --git a/spec/commands/check_spec.rb b/spec/commands/check_spec.rb index 1aa9b23..7d4afec 100644 --- a/spec/commands/check_spec.rb +++ b/spec/commands/check_spec.rb @@ -69,8 +69,9 @@ def stub_json(uri, body) stub_json( 'https://api.github.com:443/repos/jtarchie/test/pulls?direction=asc&per_page=100&sort=updated&state=open', [ - { number: 1, head: { sha: 'abcdef' } }, - { number: 2, head: { sha: 'zyxwvu' } } + { number: 1, head: { sha: 'abcdef', repo: { full_name: 'jtarchie/test' } }, base: { repo: { full_name: 'jtarchie/test' } } }, + { number: 3, head: { sha: 'ghijkl', repo: { full_name: 'jtarchie/test' } }, base: { repo: { full_name: 'jtarchie/test' } } }, + { number: 2, head: { sha: 'zyxwvu', repo: { full_name: 'someother/repo' } }, base: { repo: { full_name: 'jtarchie/test' } } }, ] ) end @@ -78,7 +79,15 @@ def stub_json(uri, body) it 'returns all PRs oldest to newest last' do expect(check('source' => { 'repo' => 'jtarchie/test', 'every' => true }, 'version' => {})).to eq [ { 'ref' => 'abcdef', 'pr' => '1' }, - { 'ref' => 'zyxwvu', 'pr' => '2' } + { 'ref' => 'ghijkl', 'pr' => '3' }, + { 'ref' => 'zyxwvu', 'pr' => '2' }, + ] + end + + it 'filters out PRs from forked repos when requested' do + expect(check('source' => { 'repo' => 'jtarchie/test', 'every' => true, 'disallow_forks' => true }, 'version' => {})).to eq [ + { 'ref' => 'abcdef', 'pr' => '1' }, + { 'ref' => 'ghijkl', 'pr' => '3' }, ] end end @@ -158,9 +167,11 @@ def stub_json(uri, body) context 'when there is more than one open pull request' do before do + stub_json('https://api.github.com/repos/jtarchie/test/statuses/abcdef', []) + stub_json('https://api.github.com/repos/jtarchie/test/statuses/zyxwvu', []) stub_json('https://api.github.com/repos/jtarchie/test/pulls?direction=asc&per_page=100&sort=updated&state=open', [ - { number: 2, head: { sha: 'zyxwvu' } }, - { number: 1, head: { sha: 'abcdef' } } + { number: 2, head: { sha: 'zyxwvu', repo: { full_name: 'someotherowner/repo' } }, base: { repo: { full_name: 'jtarchie/test' } } }, + { number: 1, head: { sha: 'abcdef', repo: { full_name: 'jtarchie/test' } }, base: { repo: { full_name: 'jtarchie/test' } } }, ]) end @@ -185,6 +196,18 @@ def stub_json(uri, body) expect(check(payload)).to eq [] end end + + context 'and `disallow_forks` is not set' do + it 'returns the most recently updated pull request' do + expect(check('source' => { 'repo' => 'jtarchie/test' })).to eq [{ 'ref' => 'zyxwvu', 'pr' => '2' }] + end + end + + context 'and `disallow_forks` is set to true' do + it 'returns the most recently updated internal pull request' do + expect(check('source' => { 'repo' => 'jtarchie/test', 'disallow_forks' => true })).to eq [{ 'ref' => 'abcdef', 'pr' => '1' }] + end + end end end end From 09fc7dd9be1139e1c76d6f2fa12c3d29acb1899a Mon Sep 17 00:00:00 2001 From: benhyland Date: Wed, 23 Nov 2016 18:04:29 +0000 Subject: [PATCH 3/3] source common.sh in check script to fix tmpdirs --- assets/check | 2 ++ 1 file changed, 2 insertions(+) diff --git a/assets/check b/assets/check index d10b27f..16db3a7 100755 --- a/assets/check +++ b/assets/check @@ -5,6 +5,8 @@ set -e exec 3>&1 # make stdout available as fd 3 for the result exec 1>&2 # redirect all output to stderr for logging +. $(dirname $0)/common.sh + payload=$(mktemp $TMPDIR/pullrequest-resource.XXXXXX) cat > $payload <&0