Add support for GitLab merge requests (MRs) #175

chamer81 · 2020-05-18T09:36:09Z

Adds an option to create a gitlab MR.

ekohl

I didn't look too closely at the code, but I wonder if we should lazy load the Gitlab gem + configure it (and the same for Github). That should keep start up time lower.

.vscode/launch.json

bin/bundle

bin/rspec

.rubocop_todo.yml

lib/modulesync.rb

bittner · 2020-05-18T12:00:26Z

Thank you @ekohl for the quick feedback. We'll take that immediately into account. We're still at fixing some rough edges of the implementation and doing internal review.

bittner · 2020-05-18T12:23:41Z

Relates to #80 (comment).

bittner · 2020-05-19T19:44:02Z

We have cleaned up the commits and I've consolidated the changes into a single commit.

There are still a few topics open: (at least from a first, quick glance)

Apart from GitLab.com, we need to allow GitLab support for private instances (config.endpoint is set for this); presumably, this needs to be configurable
The added / updated tests don't seem complete
The lazy-loading of the GitHub and GitLab dependencies is still pending realization

ekohl

I think this and the Github PR code should live in lib/modulesync/pr.rb. You'd have something like:

module ModuleSync
  class PullRequest
    def initialize(implementation, token)
      @implementation = implementation
      case @implementation
      when :github
        @client = Octokit::Client.new(:access_token => token)
      when :gitlab
        @client = Gitlab::Client.new(:private_token => token)
      else
        raise "Unknown implementation #{implementation}"
      end
    end

    def has_pr?(repo_path, source, target)
      requests = case @implementation
                 when :github
                   @client.pull_requests()
                 when :gitlab
                   @client.merge_requests()
                 end

      requests.any?
    end

    def create_pr(repo_path, source, target)
      case @implementation
      when :github
        @client.pull_request()
      when :gitlab
        @client.create_merge_request()
      end
    end

    def add_labels_to_pr(repo_path, pr, labels)
      # ....
    end
  end
end

You can also implement PullRequest and MergeRequest that implement the same interface.

This keeps lib/modulesync.rb small. It's already way too big now. An additional benefit is that you cache the client instance which I hope keeps the HTTP connection open. Not sure it does that right now.

ekohl · 2020-05-21T14:17:09Z

.gitignore

+.ruby-version
+.vscode/
+bin/bundle
+bin/rspec
+modules/
+tmp/
+vendor/


This would typically be a separate commit.

ekohl · 2020-05-21T14:17:38Z

Gemfile

@@ -2,5 +2,7 @@ source ENV['GEM_SOURCE'] || 'https://rubygems.org'

 gemspec

+gem 'aruba'


Why is this needed, looks redundant:

modulesync/modulesync.gemspec

Line 20 in 7202e6b

spec.add_development_dependency 'aruba', '~> 0.14'

ekohl · 2020-05-21T14:18:32Z

Gemfile

 gem 'cucumber', '< 3.0' if RUBY_VERSION < '2.1'
+gem 'gitlab' # gem 'gitlab', github: 'NARKOZ/gitlab'
 gem 'octokit', '~> 4.9'


This was there, but I can't explain why because this should be sufficent:

modulesync/modulesync.gemspec

Line 27 in 7202e6b

spec.add_runtime_dependency 'octokit', '~>4.0'

ekohl · 2020-05-21T14:19:32Z

Gemfile

 gem 'cucumber', '< 3.0' if RUBY_VERSION < '2.1'
+gem 'gitlab' # gem 'gitlab', github: 'NARKOZ/gitlab'


This is redundant since it's in the gemspec

mhutter · 2020-05-25T14:36:59Z

Updated the first commit to clean up dependencies in Gemfile and gemspec.
Added a second commit to refactor the existing code. This (as a side-effect) also implements lazy-loading and client reuse.
Moved the .gitignore change into its own commit

Signed-off-by: Manuel Hutter <manuel@vshn.ch>

mhutter · 2020-05-25T15:11:55Z

Still looking into that failing cucumber test, not sure how that's related...

Update: noticed that the expected outcome was changed in this PR; I reverted this (was probably changed during implemented; the original implementation of this PR might have changed error behavior)

Adds the capability of creating GitLab merge requests following the approach currently implemented for GitHub. We try to make the user experience simply by sticking to calling the feature "PR" / "--pr" also for GitLab. Which of both options, GitHub or GitLab, is chosen for processing the user request is determined by the environment variable being set: - GITHUB_TOKEN ... a PR on GitHub will be created - GITLAB_TOKEN ... a MR on GitLab will be created

* Move implementation into separate modules * Lazy-load dependencies * Reuse GitHub/GitLab clients * Add some basic specs for the GitLab implementation Signed-off-by: Manuel Hutter <manuel@vshn.ch>

This commit moves configuration validation (--pr cannot be used without --branch) into initialization. This catches the error early on, before any actual work is done. Signed-off-by: Manuel Hutter <manuel@vshn.ch>

mhutter · 2020-05-25T15:41:10Z

One thing that caught my eye when moving code around: The Gitlab implementation will ensure the configured labels are added to the PR when the PR is created, but will NOT add any labels if there is already an open PR around.

The GitHub implementation will always update the existing PR - and fail horribly from looking at the code (note this was already broken before this PR)

@ekohl what is the intended behaviour?

bittner · 2020-05-29T09:30:30Z

@ekohl Can you clarify?

ekohl

I think the current code looks like a good solution and an improvement over the current implementation. Just a small comment about the constants, otherwise 👍

I like that it only loads the Github/Gitlab code if a PR was requested which keeps the start up time lower.

ekohl · 2020-05-29T13:20:52Z

lib/modulesync.rb

 GITHUB_TOKEN = ENV.fetch('GITHUB_TOKEN', '')
-
-Octokit.configure do |c|
-  c.api_endpoint = ENV.fetch('GITHUB_BASE_URL', 'https://api.github.com')
-end
+GITLAB_TOKEN = ENV.fetch('GITLAB_TOKEN', '')


I think we can avoid these constants and only use them inline since they're only used in a single place.

Good point, totally missed it. Will adjust accordingly

ekohl · 2020-06-01T09:39:34Z

This looks good to me. It's still marked as a draft so let me know when you think it's ready to be merged.

They are not required on a global scope, so remove them from there Signed-off-by: Manuel Hutter <manuel@vshn.ch> Signed-off-by: Manuel Hutter <manuel@hutter.io>

mhutter · 2020-06-01T10:52:42Z

I broke the check in my last commit, fixed.

I think the PR is ready, can you remove the "Draft" status @chamer81 ?

I'll open a new Issue to discuss the PR label behaviour. Fixing it should be trivial.

ekohl

I'm good with this.

ekohl · 2020-06-02T12:28:25Z

Thanks!

bittner · 2020-06-02T21:43:34Z

Thanks everyone for the good collaboration! 🥇 🎊

lib/modulesync.rb

ekohl requested changes May 18, 2020

View reviewed changes

.vscode/launch.json Outdated Show resolved Hide resolved

bin/bundle Outdated Show resolved Hide resolved

bin/rspec Outdated Show resolved Hide resolved

.rubocop_todo.yml Outdated Show resolved Hide resolved

lib/modulesync.rb Outdated Show resolved Hide resolved

bittner marked this pull request as draft May 18, 2020 11:57

bittner changed the title ~~Feature/add gitlab support~~ Add support for GitLab merge requests (MRs) May 18, 2020

chamer81 force-pushed the feature/add-gitlab-support branch from db4812d to cd3ef1e Compare May 19, 2020 13:20

bastelfreak requested a review from ekohl May 19, 2020 19:30

bittner force-pushed the feature/add-gitlab-support branch from 9ee05c7 to 442c9ca Compare May 19, 2020 19:31

ekohl requested changes May 21, 2020

View reviewed changes

mhutter force-pushed the feature/add-gitlab-support branch from 442c9ca to e4ff51f Compare May 25, 2020 14:33

Extend Git ignores

4e80b0e

Signed-off-by: Manuel Hutter <manuel@vshn.ch>

mhutter force-pushed the feature/add-gitlab-support branch from e1d51b5 to d0d1ca0 Compare May 25, 2020 14:48

bittner and others added 2 commits May 25, 2020 17:18

Refactor PR/MR creation

9a0b374

* Move implementation into separate modules * Lazy-load dependencies * Reuse GitHub/GitLab clients * Add some basic specs for the GitLab implementation Signed-off-by: Manuel Hutter <manuel@vshn.ch>

mhutter force-pushed the feature/add-gitlab-support branch 2 times, most recently from a50a730 to 4b3ad12 Compare May 25, 2020 15:34

Cleanup: move config validation out of processing

a77b968

This commit moves configuration validation (--pr cannot be used without --branch) into initialization. This catches the error early on, before any actual work is done. Signed-off-by: Manuel Hutter <manuel@vshn.ch>

mhutter force-pushed the feature/add-gitlab-support branch from 4b3ad12 to a77b968 Compare May 25, 2020 15:37

ekohl reviewed May 29, 2020

View reviewed changes

Get rid of constants for PR tokens

bf27fd8

They are not required on a global scope, so remove them from there Signed-off-by: Manuel Hutter <manuel@vshn.ch> Signed-off-by: Manuel Hutter <manuel@hutter.io>

mhutter force-pushed the feature/add-gitlab-support branch from 355f85f to bf27fd8 Compare June 1, 2020 10:50

bittner marked this pull request as ready for review June 2, 2020 08:41

bittner requested a review from ekohl June 2, 2020 08:42

ekohl approved these changes Jun 2, 2020

View reviewed changes

bastelfreak approved these changes Jun 2, 2020

View reviewed changes

ekohl merged commit b590050 into voxpupuli:master Jun 2, 2020

mhutter deleted the feature/add-gitlab-support branch June 2, 2020 13:30

mhutter mentioned this pull request Jun 2, 2020

Define PR/MR label behavior #176

Open

raphink reviewed Jul 3, 2020

View reviewed changes

lib/modulesync.rb Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GitLab merge requests (MRs) #175

Add support for GitLab merge requests (MRs) #175

chamer81 commented May 18, 2020

ekohl left a comment

bittner commented May 18, 2020

bittner commented May 18, 2020

bittner commented May 19, 2020

ekohl left a comment

ekohl May 21, 2020

ekohl May 21, 2020

ekohl May 21, 2020

ekohl May 21, 2020

mhutter commented May 25, 2020 •

edited

Loading

mhutter commented May 25, 2020 •

edited

Loading

mhutter commented May 25, 2020

bittner commented May 29, 2020

ekohl left a comment

ekohl May 29, 2020

mhutter May 29, 2020

ekohl commented Jun 1, 2020

mhutter commented Jun 1, 2020

ekohl left a comment

ekohl commented Jun 2, 2020

bittner commented Jun 2, 2020

		@@ -2,5 +2,7 @@ source ENV['GEM_SOURCE'] \|\| 'https://rubygems.org'

		gemspec

		gem 'aruba'

		gem 'cucumber', '< 3.0' if RUBY_VERSION < '2.1'
		gem 'gitlab' # gem 'gitlab', github: 'NARKOZ/gitlab'

Add support for GitLab merge requests (MRs) #175

Add support for GitLab merge requests (MRs) #175

Conversation

chamer81 commented May 18, 2020

ekohl left a comment

Choose a reason for hiding this comment

bittner commented May 18, 2020

bittner commented May 18, 2020

bittner commented May 19, 2020

ekohl left a comment

Choose a reason for hiding this comment

ekohl May 21, 2020

Choose a reason for hiding this comment

ekohl May 21, 2020

Choose a reason for hiding this comment

ekohl May 21, 2020

Choose a reason for hiding this comment

ekohl May 21, 2020

Choose a reason for hiding this comment

mhutter commented May 25, 2020 • edited Loading

mhutter commented May 25, 2020 • edited Loading

mhutter commented May 25, 2020

bittner commented May 29, 2020

ekohl left a comment

Choose a reason for hiding this comment

ekohl May 29, 2020

Choose a reason for hiding this comment

mhutter May 29, 2020

Choose a reason for hiding this comment

ekohl commented Jun 1, 2020

mhutter commented Jun 1, 2020

ekohl left a comment

Choose a reason for hiding this comment

ekohl commented Jun 2, 2020

bittner commented Jun 2, 2020

mhutter commented May 25, 2020 •

edited

Loading

mhutter commented May 25, 2020 •

edited

Loading