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

#934 Refactored EndsDaemon to use ShellCommand #1004

Closed
wants to merge 2 commits into from

Conversation

original-brownbear
Copy link
Contributor

This addresses #934:

  • Created new ShellCommand class to encapsulated running a command in a shell
  • Used ShellCommand to make EndsDaemon use fewer objects and remove duplicate
    logic for sanitizing and running commands in EndsDaemon

* Created new ShellCommand class to encapsulated running a command in a shell
* Used ShellCommand to make EndsDaemon use fewer objects and remove duplicate
logic for sanitizing and running commands in EndsDaemon
@alex-palevsky
Copy link
Contributor

@original-brownbear I will find a reviewer for your pull requests shortly, thanks for contribution!

@alex-palevsky
Copy link
Contributor

@pinaf help us to review this

*
* @author Armin Braun ([email protected])
* @version $Id$
* @since 2.0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pinaf
Copy link

pinaf commented Feb 16, 2016

@original-brownbear 1 minor comment

@original-brownbear
Copy link
Contributor Author

@pinaf dealt with it :)

@pinaf
Copy link

pinaf commented Feb 16, 2016

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 16, 2016

@rultor merge

@pinaf Thanks for your request. @yegor256 Please confirm this.

@yegor256
Copy link
Owner

@rultor try to merge

@rultor
Copy link
Collaborator

rultor commented Feb 16, 2016

@rultor try to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor
Copy link
Collaborator

rultor commented Feb 16, 2016

@rultor try to merge

@yegor256 Done! FYI, the full log is here (took me 9min)

@original-brownbear
Copy link
Contributor Author

@alex-palevsky this can be closed given that it is now merged right ?

@pinaf
Copy link

pinaf commented Feb 18, 2016

@yegor256 @original-brownbear It should have been closed automatically by github after merge. Weird.

@pinaf
Copy link

pinaf commented Feb 18, 2016

@rultor deploy

@rultor
Copy link
Collaborator

rultor commented Feb 18, 2016

@rultor deploy

@pinaf Thanks for your request. @yegor256 Please confirm this.

@original-brownbear
Copy link
Contributor Author

@pinaf I think this is the case discussed here #1002.
The MR only closes when the merge could have been outright fast forwarded and the rebase Rultor does would be without effect. If there were actually new commits in between that needed to be rebased upon => hashes change => Github doesn't recognize the merge .

@alex-palevsky
Copy link
Contributor

@alex-palevsky this can be closed given that it is now merged right ?

@original-brownbear just close it yourself please

@original-brownbear
Copy link
Contributor Author

@alex-palevsky ok :)

@alex-palevsky
Copy link
Contributor

@pinaf Thanks so much! Your account was topped up for 20 mins (transaction ID is AP-4CC02639S5753930T, the task took 172 hours and 36 mins); extra minutes for review comments (c=5); +20 added to your rating, current score is: +6503

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.

5 participants