Skip to content
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

Remove the ability to run multiple commands #1752

Closed
wants to merge 2 commits into from

Conversation

jsvd
Copy link
Member

@jsvd jsvd commented Sep 18, 2014

Addresses #1747.
This removes the argument list iteration and spawning of multiple
tasks.
It's still possible to specify aditional arguments but now they're
ignored.

Addresses elastic#1747.
This removes the argument list iteration and spawning of multiple
tasks.
It's still possible to specify aditional arguments but now they're
ignored.
@jordansissel
Copy link
Contributor

I'd love to see runner be implemented as Clamp::Command with subcommands detailed the clamp way, like:

class LogStash::Runner < Clamp::Command
  subcommand "agent", ...
  subcommand "web", ...
  ...
end

@jordansissel
Copy link
Contributor

(my hope for removing the 'multi-subprocess' feature was to do a refactor that didnt' reinvent subcommand stuff)

@electrical
Copy link

Would be nice to redo it indeed in the 'right way'

@jsvd
Copy link
Member Author

jsvd commented Sep 18, 2014

Is there a benefit to Clamp once we no longer have to support "background" tasks? Maybe simple optparsing the cli options and spawing the command would be enough.

@jordansissel
Copy link
Contributor

Clamp doesn't do background tasks, but we use clamp elsewhere and it is, in my opinion, the most superior CLI library available for Ruby :)

@jordansissel
Copy link
Contributor

@electrical
Copy link

Clamp doesn't do background tasks, but we use clamp elsewhere and it is, in my opinion, the most superior CLI library available for Ruby :)

That's a discussion for an other time ;-)

@elasticsearch-release
Copy link

💚 Test passed.
Refer to this link for build results: http://build-eu-1.elasticsearch.org/job/Logstash_PR/109/

@jsvd
Copy link
Member Author

jsvd commented Sep 18, 2014

I guess I mean the Stud, specially Stud::Task. As for Clamp, no strong opinions there.

@jordansissel
Copy link
Contributor

@electrical hehe. Still! Because we support subcommands already (agent, web, irb, etc), we should probably use a library that makes it easy to do. Ruby's optparse is kind of not awesome for that.

@jordansissel
Copy link
Contributor

@jsvd +1, we don't need Stud::Task here

@jsvd
Copy link
Member Author

jsvd commented Sep 18, 2014

Well, this PR accomplished the goal of getting the ball rolling ;) Thanks for the input

@jordansissel
Copy link
Contributor

Truth! Sorry if the review I gave sounded a bit short! We can definitely do it in two PRs for sure - one to disable multiple-subproceses, and another to refactor. +1 on doing this.

@jsvd
Copy link
Member Author

jsvd commented Sep 18, 2014

I'll try to assess tomorrow if a full refactor "the clamp way" is simple/quick enough. otherwise I'd go with your two-step suggestion. 👍

@jordansissel
Copy link
Contributor

Code LGTM, mostly!

I tested it, one difference is that bin/logstash on master, with no arguments, outputs "No command given" and offers help. With this PR, it exits 0 with no output.

@jordansissel
Copy link
Contributor

Thanks for being so fast on this :)

@jsvd
Copy link
Member Author

jsvd commented Sep 18, 2014

Bugger, I'll add that test tomorrow and fix it. ty

@elasticsearch-release
Copy link

💚 Test passed.
Refer to this link for build results: http://build-eu-1.elasticsearch.org/job/Logstash_PR/110/

jsvd added a commit that referenced this pull request Sep 27, 2014
jsvd added a commit that referenced this pull request Sep 27, 2014
jsvd added a commit that referenced this pull request Sep 27, 2014
jsvd added a commit that referenced this pull request Sep 27, 2014
jsvd added a commit that referenced this pull request Sep 27, 2014
Addresses #1747.
This removes the argument list iteration and spawning of multiple
tasks.
It's still possible to specify aditional arguments but now they're
ignored.

PR: #1752
jsvd added a commit that referenced this pull request Sep 27, 2014
jsvd added a commit that referenced this pull request Sep 27, 2014
Addresses #1747.
This removes the argument list iteration and spawning of multiple
tasks.
It's still possible to specify aditional arguments but now they're
ignored.

PR: #1752
jsvd added a commit that referenced this pull request Sep 27, 2014
@jordansissel jordansissel added v1.x and removed O(2) labels Sep 27, 2014
@jordansissel jordansissel added this to the v1.5.0 milestone Sep 27, 2014
@jordansissel jordansissel self-assigned this Sep 27, 2014
@jordansissel
Copy link
Contributor

This was merged with a new (experimental) tool to help us merge a single PR into multiple branches. Hopefully it doesn't set git on fire. <3

Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants