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

Gitlab #299

Merged
merged 10 commits into from Aug 20, 2016

Conversation

Projects
None yet
8 participants
@k0nserv
Member

k0nserv commented Jul 4, 2016

Opening this PR for tracking purposes and comments from everyone else. There's still some work that needs to be finished up

  • Implement and adopt tests
  • Make sure we get a new version of the gitlab gem issued
  • For GitLab CI to work we need to convince/implement some changes in GitLab

cc @orta, @connorshea, @hjanuschka, @jk

# The title of the Pull Request
# @return String
#
def pr_title

This comment has been minimized.

@orta

orta Jul 4, 2016

Member

IMO, these should move to the gitlab vernacular - mr_title ( documentation around this will be handled fine 👍 )

@orta

orta Jul 4, 2016

Member

IMO, these should move to the gitlab vernacular - mr_title ( documentation around this will be handled fine 👍 )

This comment has been minimized.

@k0nserv

k0nserv Jul 4, 2016

Member

Agreed :)

@k0nserv

k0nserv Jul 4, 2016

Member

Agreed :)

This comment has been minimized.

@k0nserv

k0nserv Jul 4, 2016

Member

Unless there's a need for a common interface

@k0nserv

k0nserv Jul 4, 2016

Member

Unless there's a need for a common interface

This comment has been minimized.

@orta

orta Jul 4, 2016

Member

IMO, I don't think so, for Danger::RequestSources::GitLab there should be, ( you could add method aliases though, so that people can C&P examples that may reference the github names? I'd be on board with that. )

@orta

orta Jul 4, 2016

Member

IMO, I don't think so, for Danger::RequestSources::GitLab there should be, ( you could add method aliases though, so that people can C&P examples that may reference the github names? I'd be on board with that. )

This comment has been minimized.

@k0nserv

k0nserv Jul 4, 2016

Member

Yeah it's only used by the matching request source as of now so

@k0nserv

k0nserv Jul 4, 2016

Member

Yeah it's only used by the matching request source as of now so

Show outdated Hide outdated lib/danger/request_source/github.rb
module Danger
module RequestSources
class GitHub < RequestSource
include Danger::Helpers::CommentsHelper

This comment has been minimized.

@orta

orta Jul 4, 2016

Member

👍

@orta

orta Jul 4, 2016

Member

👍

@DangerCI

This comment has been minimized.

Show comment
Hide comment
@DangerCI

DangerCI Jul 4, 2016

3 Errors
🚫 Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.
🚫 Failing due to documentation issues, see below.
🚫 The core plugins are not at 100% doc’d - see below:
Please include a CHANGELOG entry. You can find it at CHANGELOG.md.
Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

1 Warning
⚠️ .gemspec modified
External contributor has edited the Gemspec
Nice work.
@k0nserv is not a contributor yet, would you like to join the Danger org?

Core Docs Errors

[!] Failed

Errors

Warnings

Files:           5
Modules:         1 (    0 undocumented)
Classes:         5 (    1 undocumented)
Constants:       0 (    0 undocumented)
Attributes:      0 (    0 undocumented)
Methods:        48 (    2 undocumented)
 94.44% documented

Undocumented Objects:

(in file: lib/danger/danger_core/plugins/dangerfile_gitlab_plugin.rb)
Danger::DangerfileGitLabPlugin
Danger::DangerfileGitLabPlugin.new
Danger::DangerfileGitLabPlugin.instance_name

Generated by 🚫 danger

DangerCI commented Jul 4, 2016

3 Errors
🚫 Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.
🚫 Failing due to documentation issues, see below.
🚫 The core plugins are not at 100% doc’d - see below:
Please include a CHANGELOG entry. You can find it at CHANGELOG.md.
Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

1 Warning
⚠️ .gemspec modified
External contributor has edited the Gemspec
Nice work.
@k0nserv is not a contributor yet, would you like to join the Danger org?

Core Docs Errors

[!] Failed

Errors

Warnings

Files:           5
Modules:         1 (    0 undocumented)
Classes:         5 (    1 undocumented)
Constants:       0 (    0 undocumented)
Attributes:      0 (    0 undocumented)
Methods:        48 (    2 undocumented)
 94.44% documented

Undocumented Objects:

(in file: lib/danger/danger_core/plugins/dangerfile_gitlab_plugin.rb)
Danger::DangerfileGitLabPlugin
Danger::DangerfileGitLabPlugin.new
Danger::DangerfileGitLabPlugin.instance_name

Generated by 🚫 danger

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 4, 2016

Member

On the topic of the gitlab gem I've reached out to Narkos over email. He is the registered owner of the gem on https://rubygems.org. Issuing a 3.7.0 of gitlab is currently the largest blocker for this to be merged

Member

k0nserv commented Jul 4, 2016

On the topic of the gitlab gem I've reached out to Narkos over email. He is the registered owner of the gem on https://rubygems.org. Issuing a 3.7.0 of gitlab is currently the largest blocker for this to be merged

@orta orta referenced this pull request Jul 7, 2016

Closed

Bitbucket integration #315

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 7, 2016

Member

@k0nserv any reply from Narkos?

Member

orta commented Jul 7, 2016

@k0nserv any reply from Narkos?

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 7, 2016

Member

No word from Narkos yet. You wanna try the Rubygems route?

Member

k0nserv commented Jul 7, 2016

No word from Narkos yet. You wanna try the Rubygems route?

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 7, 2016

Member

Might be possible to convince them to give @asedge push access

Member

k0nserv commented Jul 7, 2016

Might be possible to convince them to give @asedge push access

@hjanuschka

This comment has been minimized.

Show comment
Hide comment
@hjanuschka

hjanuschka Jul 7, 2016

i would be scared if it would work!

how about forking it to gitlab-official or gitlab-gitlab?

i would be scared if it would work!

how about forking it to gitlab-official or gitlab-gitlab?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 7, 2016

Member

I think it's better we see if we can get either the attention of asedge, or someone who works at GitLab, it's a pretty clear-cut choice to say that they should have access in the absence of Narkos.

Member

orta commented Jul 7, 2016

I think it's better we see if we can get either the attention of asedge, or someone who works at GitLab, it's a pretty clear-cut choice to say that they should have access in the absence of Narkos.

@connorshea

This comment has been minimized.

Show comment
Hide comment
@connorshea

connorshea Jul 7, 2016

Contributor

I'm undecided as to whether we should try to get control of the gem or create a new one, I've asked fellow GitLabbers for their input.

It'd also be good to have your thoughts, @asedge, seeing as you're the primary maintainer currently.

Contributor

connorshea commented Jul 7, 2016

I'm undecided as to whether we should try to get control of the gem or create a new one, I've asked fellow GitLabbers for their input.

It'd also be good to have your thoughts, @asedge, seeing as you're the primary maintainer currently.

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 7, 2016

Member

@orta Do you wanna exclude GitLab CI on account of it not being supported as of now?

Member

k0nserv commented Jul 7, 2016

@orta Do you wanna exclude GitLab CI on account of it not being supported as of now?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 7, 2016

Member

You mean merge this, but not let the code-paths be accessible? That can work for me

Member

orta commented Jul 7, 2016

You mean merge this, but not let the code-paths be accessible? That can work for me

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 7, 2016

Member

Either that or not merge it at all. Until they support some way of exposing the MR id in GitLab CI Danger won't work at all, even after that they need to support triggering builds on comments/edits for full support

Member

k0nserv commented Jul 7, 2016

Either that or not merge it at all. Until they support some way of exposing the MR id in GitLab CI Danger won't work at all, even after that they need to support triggering builds on comments/edits for full support

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 7, 2016

Member

All though if we include it as it stands right now people are free to build small servers that interact with GitLab's build hooks to expose the correct env variables

Member

k0nserv commented Jul 7, 2016

All though if we include it as it stands right now people are free to build small servers that interact with GitLab's build hooks to expose the correct env variables

@justMaku

This comment has been minimized.

Show comment
Hide comment
@justMaku

justMaku Jul 7, 2016

Member

I'd merge it and allow people to build their own workflows, even if GitLab CI doesn't work now, people can still use Jenkins or any other form of CI to trigger the build.

Member

justMaku commented Jul 7, 2016

I'd merge it and allow people to build their own workflows, even if GitLab CI doesn't work now, people can still use Jenkins or any other form of CI to trigger the build.

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 7, 2016

Member

The question is not about other CI tools though, it's specifically about Danger::CISource::GitlabCI.

Member

k0nserv commented Jul 7, 2016

The question is not about other CI tools though, it's specifically about Danger::CISource::GitlabCI.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 7, 2016

Member

Aye, I'm on the let's merge it, but comment out the Danger::CISource::GitlabCI - it'll end up in the autogenerated docs on the website

Member

orta commented Jul 7, 2016

Aye, I'm on the let's merge it, but comment out the Danger::CISource::GitlabCI - it'll end up in the autogenerated docs on the website

@justMaku

This comment has been minimized.

Show comment
Hide comment
@justMaku

justMaku Jul 7, 2016

Member

I also believe that assuming that Danger only works on PRs is wrong as in fact danger could work on any given changeset. We need to start thinking about removing that tight coupling somehow and maybe even move out PR updaters to a plugin.

Member

justMaku commented Jul 7, 2016

I also believe that assuming that Danger only works on PRs is wrong as in fact danger could work on any given changeset. We need to start thinking about removing that tight coupling somehow and maybe even move out PR updaters to a plugin.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 7, 2016

Member

I also believe that assuming that Danger only works on PRs is wrong as in fact danger could work on any given changeset.

Could, yes, I guess I can see some usage as a pre-linter, but it might not be worth the overhead for support.

Member

orta commented Jul 7, 2016

I also believe that assuming that Danger only works on PRs is wrong as in fact danger could work on any given changeset.

Could, yes, I guess I can see some usage as a pre-linter, but it might not be worth the overhead for support.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 27, 2016

Member

We may need to fork the gem, I don't want this PR sitting idle for this long

Member

orta commented Jul 27, 2016

We may need to fork the gem, I don't want this PR sitting idle for this long

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 27, 2016

Member

It's not unheard of, @KrauseFx had to do it for shenzhen - https://github.com/fastlane/shenzhen

Member

orta commented Jul 27, 2016

It's not unheard of, @KrauseFx had to do it for shenzhen - https://github.com/fastlane/shenzhen

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 27, 2016

Member

Yes I agree that might be the best course of action. I've been ill for a few weeks so haven't really had the time and energy to work on this unfortunately.

Member

k0nserv commented Jul 27, 2016

Yes I agree that might be the best course of action. I've been ill for a few weeks so haven't really had the time and energy to work on this unfortunately.

@champo

This comment has been minimized.

Show comment
Hide comment
@champo

champo Jul 27, 2016

Member

wouldn't it make sense to build this PR in two parts? One preparing danger to go multi source and then another adding GitLab? Some of the changes in this PR would be really nice to have even if GitLab doesn't release the gem.

Member

champo commented Jul 27, 2016

wouldn't it make sense to build this PR in two parts? One preparing danger to go multi source and then another adding GitLab? Some of the changes in this PR would be really nice to have even if GitLab doesn't release the gem.

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 27, 2016

Member

Yeah I was thinking about extracting

into a separate PR because they can all be merged and makes supporting multiple sources easier and can provide value right away

Member

k0nserv commented Jul 27, 2016

Yeah I was thinking about extracting

into a separate PR because they can all be merged and makes supporting multiple sources easier and can provide value right away

@connorshea

This comment has been minimized.

Show comment
Hide comment
@connorshea

connorshea Jul 29, 2016

Contributor

@k0nserv wanted to check up on this, besides the gitlab gem, what else is blocking this?

(Apologies if this was already asked and I forgot 🙈)

Contributor

connorshea commented Jul 29, 2016

@k0nserv wanted to check up on this, besides the gitlab gem, what else is blocking this?

(Apologies if this was already asked and I forgot 🙈)

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Jul 29, 2016

Member

@connorshea For this PR specifically the test for Danger::RequestSource::GitLab still need to be written which is the work I've been unable to do due to being ill the past few weeks. From GitLab's side it's still interesting to explore the blockers for GitLab CI, but since there are other CI providers this change can still go into Danger before that

Member

k0nserv commented Jul 29, 2016

@connorshea For this PR specifically the test for Danger::RequestSource::GitLab still need to be written which is the work I've been unable to do due to being ill the past few weeks. From GitLab's side it's still interesting to explore the blockers for GitLab CI, but since there are other CI providers this change can still go into Danger before that

@deanpcmad

This comment has been minimized.

Show comment
Hide comment
@deanpcmad

deanpcmad Aug 6, 2016

Contributor

This looks good 👍 Any update so far? I would like to use Danger for all my projects 😄

Contributor

deanpcmad commented Aug 6, 2016

This looks good 👍 Any update so far? I would like to use Danger for all my projects 😄

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 6, 2016

Member

It's looking like it needs some tests, and for a decision around whether we have to have our own fork of the gitlab gem in order to accommodate the changes.

Another option, is that we say it can only be used via a Gemfile with the reference to master for the moment. Shipped with caveats is better than uncertainty.

Member

orta commented Aug 6, 2016

It's looking like it needs some tests, and for a decision around whether we have to have our own fork of the gitlab gem in order to accommodate the changes.

Another option, is that we say it can only be used via a Gemfile with the reference to master for the moment. Shipped with caveats is better than uncertainty.

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 6, 2016

Member

Yeah still needs more tests, but the most uncertain outstanding question is around the gitlab gem. I raised the question about ownership of the gems with rubygems last week, but I am yet to hear from them. I'd suggest we wait and see if the problem can't be resolved by promoting one of the existing contributors. If that's not possible we'll have to fork it

Member

k0nserv commented Aug 6, 2016

Yeah still needs more tests, but the most uncertain outstanding question is around the gitlab gem. I raised the question about ownership of the gems with rubygems last week, but I am yet to hear from them. I'd suggest we wait and see if the problem can't be resolved by promoting one of the existing contributors. If that's not possible we'll have to fork it

@deanpcmad

This comment has been minimized.

Show comment
Hide comment
@deanpcmad

deanpcmad Aug 6, 2016

Contributor

Ah ok, I'll see if I can copy the GH tests and get them working. Can my commits be added to this PR or would I need to create a separate one?

Contributor

deanpcmad commented Aug 6, 2016

Ah ok, I'll see if I can copy the GH tests and get them working. Can my commits be added to this PR or would I need to create a separate one?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 6, 2016

Member

@deanpcmad it would need to go to this branch on @k0nserv's repo - https://github.com/k0nserv/danger/tree/gitlab

Member

orta commented Aug 6, 2016

@deanpcmad it would need to go to this branch on @k0nserv's repo - https://github.com/k0nserv/danger/tree/gitlab

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 6, 2016

Member

I can handle a lot of the documentation issues that Danger is raising too, post-merge.

Member

orta commented Aug 6, 2016

I can handle a lot of the documentation issues that Danger is raising too, post-merge.

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 6, 2016

Member

Sure thing @deanpcmad you're welcome to take a stab at finishing up the tests. I've invited you to my fork so you can push to this branch.

Member

k0nserv commented Aug 6, 2016

Sure thing @deanpcmad you're welcome to take a stab at finishing up the tests. I've invited you to my fork so you can push to this branch.

@deanpcmad

This comment has been minimized.

Show comment
Hide comment
@deanpcmad

deanpcmad Aug 6, 2016

Contributor

I've fixed a couple of tests. I can't work out why the EnvironmentManager tests fail though...

Contributor

deanpcmad commented Aug 6, 2016

I've fixed a couple of tests. I can't work out why the EnvironmentManager tests fail though...

Show outdated Hide outdated lib/danger/ci_source/gitlab.rb
def self.validates?(env)
return !env["GITLAB_PROJECT_ID"].nil? && !env["GITLAB_MR_ID"].nil?
end

This comment has been minimized.

@orta

orta Aug 6, 2016

Member

These validates_as won't be hooked up correctly, they'll need porting to self.validates_as_ci? and self.validates_as_pr?

The first one is when you're sure that it's GitLabCI, the second is when you're sure it's actually a PR/MR

@orta

orta Aug 6, 2016

Member

These validates_as won't be hooked up correctly, they'll need porting to self.validates_as_ci? and self.validates_as_pr?

The first one is when you're sure that it's GitLabCI, the second is when you're sure it's actually a PR/MR

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 7, 2016

Member

There's another meta problem with GitLab that might be good to document somewhere. From my understanding GitHub and GitLab differs around builds with forks. In GitHub with Travis when you submit a PR from a fork it's built under the namespace of the original project. For example this PR is from my fork, but builds are still build under danger's travis namespace.

On GitLab the default behaviour of forks is that nothing happens unless the fork sets up CI as well. There are open issues for GitLab about changing this behaviour.

EDIT: At least this is my experience with GitLab CI + Drone.

Member

k0nserv commented Aug 7, 2016

There's another meta problem with GitLab that might be good to document somewhere. From my understanding GitHub and GitLab differs around builds with forks. In GitHub with Travis when you submit a PR from a fork it's built under the namespace of the original project. For example this PR is from my fork, but builds are still build under danger's travis namespace.

On GitLab the default behaviour of forks is that nothing happens unless the fork sets up CI as well. There are open issues for GitLab about changing this behaviour.

EDIT: At least this is my experience with GitLab CI + Drone.

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 7, 2016

Member

Found an old issue and for this on GitHub and reopened it on GitLab

Member

k0nserv commented Aug 7, 2016

Found an old issue and for this on GitHub and reopened it on GitLab

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 12, 2016

Member

So I was working on this and found another issue.

GitLab seems to be stripping out the data attributes from the submitted HTML :(

Member

k0nserv commented Aug 12, 2016

So I was working on this and found another issue.

GitLab seems to be stripping out the data attributes from the submitted HTML :(

@connorshea

This comment has been minimized.

Show comment
Hide comment
@connorshea

connorshea Aug 12, 2016

Contributor

@deanpcmad thanks ❤️ Conflicts on that MR

Contributor

connorshea commented Aug 12, 2016

@deanpcmad thanks ❤️ Conflicts on that MR

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 12, 2016

Member

GitLab seems to be stripping out the data attributes from the submitted HTML :(

Interesting, when I C&P'd the danger HTML into an issue the raw text kept the attributes:

screen shot 2016-08-12 at 3 14 44 pm

However the rendered markdown doesn't. I would guess that Danger is reading the markdown, so that may be ok?

Member

orta commented Aug 12, 2016

GitLab seems to be stripping out the data attributes from the submitted HTML :(

Interesting, when I C&P'd the danger HTML into an issue the raw text kept the attributes:

screen shot 2016-08-12 at 3 14 44 pm

However the rendered markdown doesn't. I would guess that Danger is reading the markdown, so that may be ok?

@connorshea

This comment has been minimized.

Show comment
Hide comment
@connorshea

connorshea Aug 12, 2016

Contributor

@k0nserv @orta does GitHub not strip out the data attributes? I'm looking at the Danger table in this thread and there don't seem to be any data attributes.

Contributor

connorshea commented Aug 12, 2016

@k0nserv @orta does GitHub not strip out the data attributes? I'm looking at the Danger table in this thread and there don't seem to be any data attributes.

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 12, 2016

Member

I was only looking at the frontend, might just be the case that it happends during rendering yes

Member

k0nserv commented Aug 12, 2016

I was only looking at the frontend, might just be the case that it happends during rendering yes

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 12, 2016

Member

Yeah my bad it's in the frontend rendering

Member

k0nserv commented Aug 12, 2016

Yeah my bad it's in the frontend rendering

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 14, 2016

Member

@deanpcmad Do you have any WIP for this branch? If not I've finished the tests and rebased the branch so if you are cool with it I'll force push those changes

Member

k0nserv commented Aug 14, 2016

@deanpcmad Do you have any WIP for this branch? If not I've finished the tests and rebased the branch so if you are cool with it I'll force push those changes

@deanpcmad

This comment has been minimized.

Show comment
Hide comment
@deanpcmad

deanpcmad Aug 14, 2016

Contributor

@k0nserv go ahead :)

Contributor

deanpcmad commented Aug 14, 2016

@k0nserv go ahead :)

RSpec.configure do |config|
config.filter_gems_from_backtrace "bundler"
config.include Danger::Support::GitLabHelper, host: :gitlab
config.include Danger::Support::GitHubHelper, host: :github

This comment has been minimized.

@k0nserv

k0nserv Aug 14, 2016

Member

I've reverted back to this method of controlling the environment in tests. I believe it's much cleaner and more in line with rspec

@k0nserv

k0nserv Aug 14, 2016

Member

I've reverted back to this method of controlling the environment in tests. I believe it's much cleaner and more in line with rspec

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 14, 2016

Member

This will require quite some cleaning up among the commits, but I'll do that as the very last thing. Still have a few more tests to write, but for the gitlab request source we now have 100% coverage.

There's also the question of organization danger files which isn't implemented yet

Member

k0nserv commented Aug 14, 2016

This will require quite some cleaning up among the commits, but I'll do that as the very last thing. Still have a few more tests to write, but for the gitlab request source we now have 100% coverage.

There's also the question of organization danger files which isn't implemented yet

@deanpcmad

This comment has been minimized.

Show comment
Hide comment
@deanpcmad

deanpcmad Aug 14, 2016

Contributor

👍 looking forward to adding this to all my tests now 😃

Contributor

deanpcmad commented Aug 14, 2016

👍 looking forward to adding this to all my tests now 😃

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 16, 2016

Member

There's also the question of organization danger files which isn't implemented yet

Don't do this ATM, I've already removed it in one PR - #418 - I'm not sure what its replacement should look like yet

Member

orta commented Aug 16, 2016

There's also the question of organization danger files which isn't implemented yet

Don't do this ATM, I've already removed it in one PR - #418 - I'm not sure what its replacement should look like yet

@connorshea

This comment has been minimized.

Show comment
Hide comment
@connorshea

connorshea Aug 16, 2016

Contributor

Hey all, we've gotten the current maintainer of the GitLab gem able to ship new releases! 3.7.0 is now out! https://rubygems.org/gems/gitlab

Contributor

connorshea commented Aug 16, 2016

Hey all, we've gotten the current maintainer of the GitLab gem able to ship new releases! 3.7.0 is now out! https://rubygems.org/gems/gitlab

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 17, 2016

Member

🎉

Member

orta commented Aug 17, 2016

🎉

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 20, 2016

Member

I believe this should be ready for review now. I haven't written any code to update the status of the merge request yet, but I'd prefer to merge first and then add this

Member

k0nserv commented Aug 20, 2016

I believe this should be ready for review now. I haven't written any code to update the status of the merge request yet, but I'd prefer to merge first and then add this

@k0nserv

This comment has been minimized.

Show comment
Hide comment
@k0nserv

k0nserv Aug 20, 2016

Member

The other thing that remains is updating CI sources that support GitLab. I don't believe this to be a blocker for the merge either

Member

k0nserv commented Aug 20, 2016

The other thing that remains is updating CI sources that support GitLab. I don't believe this to be a blocker for the merge either

# The new method uses a more robust data-danger-table
# tag instead.
match = GITHUB_OLD_REGEX.match(table)
return match if match

This comment has been minimized.

@orta

orta Aug 20, 2016

Member

👍

@orta

orta Aug 20, 2016

Member

👍

:should_remove_source_branch, :force_remove_source_branch
].each do |key|
key_present = plugin.pr_json.key?(key.to_s)
expect(key_present).to be_truthy, "Expected key #{key} not found"

This comment has been minimized.

@orta

orta Aug 20, 2016

Member

Nitpick: This might be over-testing, you're mostly testing the json parser here, and as the API changes - the expectations in this test will become out of sync with what the stubbed JSON says are reality

@orta

orta Aug 20, 2016

Member

Nitpick: This might be over-testing, you're mostly testing the json parser here, and as the API changes - the expectations in this test will become out of sync with what the stubbed JSON says are reality

This comment has been minimized.

@orta

orta Aug 20, 2016

Member

It definitely looks cool though

@orta

orta Aug 20, 2016

Member

It definitely looks cool though

describe Danger::RequestSources::GitLab, host: :gitlab do
let(:env) { stub_env }
let(:g) { Danger::RequestSources::GitLab.new(stub_ci, env) }

This comment has been minimized.

@orta

orta Aug 20, 2016

Member

This let helper is nice

@orta

orta Aug 20, 2016

Member

This let helper is nice

File.open(dir + "/file2", "w") {}
`git add .`
`git commit -m "another"`
`git remote add origin git@gitlab.com:k0nserv/danger-test.git`

This comment has been minimized.

@orta

orta Aug 20, 2016

Member

👍

@orta

orta Aug 20, 2016

Member

👍

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 20, 2016

Member

OK, so yeah, this looks good, I have only minor nitpicks about the tests - but that's not a blocker for me at all. I'm going to merge this into another branch on danger ( I want to be able to deploy master whenever ) so I can get this rebased, documented and green. A new PR will be coming in soon, once I get back to my main computer.

This has been a bunch of work from a quite a few developers, thanks to all of you. 💯

Member

orta commented Aug 20, 2016

OK, so yeah, this looks good, I have only minor nitpicks about the tests - but that's not a blocker for me at all. I'm going to merge this into another branch on danger ( I want to be able to deploy master whenever ) so I can get this rebased, documented and green. A new PR will be coming in soon, once I get back to my main computer.

This has been a bunch of work from a quite a few developers, thanks to all of you. 💯

@orta orta changed the base branch from master to gitlab_docs Aug 20, 2016

@orta orta merged commit 5f2572c into danger:gitlab_docs Aug 20, 2016

1 of 3 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@hjanuschka

This comment has been minimized.

Show comment
Hide comment
@hjanuschka

hjanuschka Aug 21, 2016

nice thx guys!!!

nice thx guys!!!

@k0nserv k0nserv deleted the k0nserv:gitlab branch Aug 21, 2016

@k0nserv k0nserv changed the title from [WIP] Gitlab to Gitlab Aug 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment