From 4259334fb64ef49580e58d434bc63d3c4738a77c Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Thu, 27 Oct 2016 15:07:18 +0200 Subject: [PATCH 1/2] Fix applying labels for GitHub-imported MRs --- CHANGELOG.md | 1 + lib/gitlab/github_import/client.rb | 12 +++++++----- lib/gitlab/github_import/importer.rb | 11 +++++++++-- spec/lib/gitlab/github_import/importer_spec.rb | 1 + 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff8e03e7d58..6bf63582cbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ Please view this file on the master branch, on stable branches it's out of date. - Fix Sign in page 'Forgot your password?' link overlaps on medium-large screens - Show full status link on MR & commit pipelines - Fix documents and comments on Build API `scope` + - Fix applying labels for GitHub-imported MRs - Refactor email, use setter method instead AR callbacks for email attribute (Semyon Pupkov) ## 8.13.1 (2016-10-25) diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index 7f424b74efb..348005d5659 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -105,18 +105,20 @@ module Gitlab data = api.send(method, *args) return data unless data.is_a?(Array) + last_response = api.last_response + if block_given? yield data - each_response_page(&block) + # api.last_response could change while we're yielding (e.g. fetching labels for each PR) + # so we cache our own last request + each_response_page(last_response, &block) else - each_response_page { |page| data.concat(page) } + each_response_page(last_response) { |page| data.concat(page) } data end end - def each_response_page - last_response = api.last_response - + def each_response_page(last_response) while last_response.rels[:next] sleep rate_limit_sleep_time if rate_limit_exceed? last_response = last_response.rels[:next].get diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 4b70f33a851..4ac932dc213 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -132,8 +132,15 @@ module Gitlab end def apply_labels(issuable, raw_issuable) - if raw_issuable.labels.count > 0 - label_ids = raw_issuable.labels + # GH returns labels for issues but not for pull requests! + labels = if issuable.is_a?(MergeRequest) + client.labels_for_issue(repo, raw_issuable.number) + else + raw_issuable.labels + end + + if labels.count > 0 + label_ids = labels .map { |attrs| @labels[attrs.name] } .compact diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb index 1af553f8f03..84f280ceaae 100644 --- a/spec/lib/gitlab/github_import/importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer_spec.rb @@ -154,6 +154,7 @@ describe Gitlab::GithubImport::Importer, lib: true do { type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" }, { type: :milestone, url: "https://api.github.com/repos/octocat/Hello-World/milestones/1", errors: "Validation failed: Title has already been taken" }, { type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank, Title is too short (minimum is 0 characters)" }, + { type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Invalid Repository. Use user/repo format." }, { type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Validation failed: Validate branches Cannot Create: This merge request already exists: [\"New feature\"]" }, { type: :wiki, errors: "Gitlab::Shell::Error" }, { type: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" } From ce38ae8ca15714e710c5198d201336f3651ad788 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Thu, 27 Oct 2016 15:08:37 +0200 Subject: [PATCH 2/2] Fix importing MR comments from GitHub --- CHANGELOG.md | 1 + lib/gitlab/github_import/importer.rb | 13 +++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bf63582cbd..1cd16ca4ccd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ Please view this file on the master branch, on stable branches it's out of date. - Show full status link on MR & commit pipelines - Fix documents and comments on Build API `scope` - Fix applying labels for GitHub-imported MRs + - Fix importing MR comments from GitHub - Refactor email, use setter method instead AR callbacks for email attribute (Semyon Pupkov) ## 8.13.1 (2016-10-25) diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index 4ac932dc213..27946dff608 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -150,21 +150,22 @@ module Gitlab def import_comments client.issues_comments(repo, per_page: 100) do |comments| - create_comments(comments, :issue) + create_comments(comments) end client.pull_requests_comments(repo, per_page: 100) do |comments| - create_comments(comments, :pull_request) + create_comments(comments) end end - def create_comments(comments, issuable_type) + def create_comments(comments) ActiveRecord::Base.no_touching do comments.each do |raw| begin - comment = CommentFormatter.new(project, raw) - issuable_class = issuable_type == :issue ? Issue : MergeRequest - iid = raw.send("#{issuable_type}_url").split('/').last # GH doesn't return parent ID directly + comment = CommentFormatter.new(project, raw) + # GH does not return info about comment's parent, so we guess it by checking its URL! + *_, parent, iid = URI(raw.html_url).path.split('/') + issuable_class = parent == 'issues' ? Issue : MergeRequest issuable = issuable_class.find_by_iid(iid) next unless issuable