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

[Ready] Remote plugins #156

Merged
merged 27 commits into from Apr 6, 2016

Conversation

Projects
None yet
4 participants
@KrauseFx
Member

KrauseFx commented Mar 24, 2016

Working on local and remote actions using simple Ruby classes (no gems)

Open points:

  • Update plugins to be classes and not methods
  • danger new_plugin command
  • Tests

@KrauseFx KrauseFx self-assigned this Mar 24, 2016

KrauseFx added some commits Mar 24, 2016

Upgraded plugins to be fully featured classes
Added automatic redirects of calls for those

@KrauseFx KrauseFx changed the title from [WIP] Remote plugins to [Ready] Remote plugins Mar 25, 2016

@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Mar 25, 2016

Member

This PR is ready for review. Circle is failing for a random bundler reason.
I don't know why Travis fails with

lib/assets/ActionTemplate.rb:4:28: E: unexpected token tLT
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
      class [[CLASS_NAME]] < Plugin
                           ^
lib/assets/ActionTemplate.rb:20:1: E: unexpected token kEND
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
end
^^^
44 files inspected, 2 offenses detected
RuboCop failed!

This file is excluded in .rubocop.yml and works locally. Any ideas?

Member

KrauseFx commented Mar 25, 2016

This PR is ready for review. Circle is failing for a random bundler reason.
I don't know why Travis fails with

lib/assets/ActionTemplate.rb:4:28: E: unexpected token tLT
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
      class [[CLASS_NAME]] < Plugin
                           ^
lib/assets/ActionTemplate.rb:20:1: E: unexpected token kEND
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
end
^^^
44 files inspected, 2 offenses detected
RuboCop failed!

This file is excluded in .rubocop.yml and works locally. Any ideas?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Mar 25, 2016

Member

@dbgrandi - I'll let you review etc

I don't think danger new_plugin should exist though, it should be replaced by your danger plugins create is that close yet?

Member

orta commented Mar 25, 2016

@dbgrandi - I'll let you review etc

I don't think danger new_plugin should exist though, it should be replaced by your danger plugins create is that close yet?

Show outdated Hide outdated CHANGELOG.md
@@ -3,6 +3,7 @@
require "danger/environment_manager"
require "danger/commands/runner"
require "danger/available_values"
require "danger/core_ext/string"

This comment has been minimized.

@dbgrandi

dbgrandi Mar 25, 2016

Member

Are these in a particular order? The ones below are all 🔤

@dbgrandi

dbgrandi Mar 25, 2016

Member

Are these in a particular order? The ones below are all 🔤

This comment has been minimized.

@KrauseFx

KrauseFx Mar 28, 2016

Member

That shouldn't really matter

@KrauseFx

KrauseFx Mar 28, 2016

Member

That shouldn't really matter

@dbgrandi

This comment has been minimized.

Show comment
Hide comment
@dbgrandi

dbgrandi Mar 25, 2016

Member

Looks good! 🔭

I've still got a bunch to do on refactoring for the plugin manager.

Member

dbgrandi commented Mar 25, 2016

Looks good! 🔭

I've still got a bunch to do on refactoring for the plugin manager.

Show outdated Hide outdated README.md
@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Mar 31, 2016

Member

Okay, addressed all the feedback and updated the PR, thanks everyone.

@orta Do you want danger new_plugin to be changed to danger plugins create? If so, could you point me to the right direction on how to implement this?

Member

KrauseFx commented Mar 31, 2016

Okay, addressed all the feedback and updated the PR, thanks everyone.

@orta Do you want danger new_plugin to be changed to danger plugins create? If so, could you point me to the right direction on how to implement this?

@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Mar 31, 2016

Member

@dbgrandi @jeroenvisser101 I'd appreciated another review of the changes 👍

Member

KrauseFx commented Mar 31, 2016

@dbgrandi @jeroenvisser101 I'd appreciated another review of the changes 👍

@jeroenvisser101

This comment has been minimized.

Show comment
Hide comment
@jeroenvisser101
Member

jeroenvisser101 commented Mar 31, 2016

@KrauseFx LGTM! 🚀

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Mar 31, 2016

Member

Do you want danger new_plugin to be changed to danger plugins create

No, we'll have a real one coming in one of these days with #131, I'd recommend dropping it from this PR

Member

orta commented Mar 31, 2016

Do you want danger new_plugin to be changed to danger plugins create

No, we'll have a real one coming in one of these days with #131, I'd recommend dropping it from this PR

@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Apr 1, 2016

Member

@orta I should completely remove the danger new_plugin command? How are people supposed to create new local plugins? This is different from #131 as it's mostly used for local additions to danger

Member

KrauseFx commented Apr 1, 2016

@orta I should completely remove the danger new_plugin command? How are people supposed to create new local plugins? This is different from #131 as it's mostly used for local additions to danger

@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Apr 5, 2016

Member

@orta just wanted to ask you about the last remaining open point #156 (comment)

Member

KrauseFx commented Apr 5, 2016

@orta just wanted to ask you about the last remaining open point #156 (comment)

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Apr 5, 2016

Member

Keep it in, but don't be surprised if it gets removed once we can push gems properly

Member

orta commented Apr 5, 2016

Keep it in, but don't be surprised if it gets removed once we can push gems properly

Merge branch 'master' into remote-plugins
# Conflicts:
#	Dangerfile
#	README.md
#	lib/danger/dangerfile_dsl.rb
@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Apr 5, 2016

Member

Alright, resolved merge conflicts - working on finishing things up now 👍

Member

KrauseFx commented Apr 5, 2016

Alright, resolved merge conflicts - working on finishing things up now 👍

@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Apr 5, 2016

Member

I pushed a few more commits with more tests and documentation, they should be live soon, just waiting for https://twitter.com/githubstatus/status/717464295454470145.

Ready to merge from my side. Tests are passing 👍

Member

KrauseFx commented Apr 5, 2016

I pushed a few more commits with more tests and documentation, they should be live soon, just waiting for https://twitter.com/githubstatus/status/717464295454470145.

Ready to merge from my side. Tests are passing 👍

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Apr 6, 2016

Member

Let's do it! 👍

Member

orta commented Apr 6, 2016

Let's do it! 👍

@orta orta merged commit 4a2d257 into master Apr 6, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@orta orta deleted the remote-plugins branch Apr 6, 2016

@KrauseFx

This comment has been minimized.

Show comment
Hide comment
@KrauseFx

KrauseFx Apr 6, 2016

Member

Oh yeah, thanks for merging @orta 👍

Member

KrauseFx commented Apr 6, 2016

Oh yeah, thanks for merging @orta 👍

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