Add `--no-hooks` option for running commands without `before()` and `after()` (#934) #1061

pluseg · 2017-03-03T01:52:40Z

Q	A
Bug fix?	No
New feature?	Yes
BC breaks?	No
Deprecations?	Yes
Fixed tickets	N/A

Updated version of #934.

…after()` (deployphp#934)

pluseg · 2017-03-03T02:10:36Z

src/Task/GroupTask.php

-     * List of tasks names.
+     * List of tasks names with before and after hooks.
+     *
+     * @deprecated v5.0


I offer deprecate this as useless.

pluseg · 2017-03-03T02:12:33Z

src/Task/Task.php

@@ -227,6 +227,10 @@ public function setPrivate()
     */
    public function addBefore($task)
    {
+        if (!is_string($task)) {
+            throw new \RuntimeException('Invalid argument to `before` hook of "' . $this->getName() . '" task.');
+        }


I mentioned that there is no check that $task is a string.

When it should be an InvalidArgumentException.

antonmedv · 2017-03-03T05:38:42Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 ### Added
 - Add a way to retrieve a defined task [#1008](https://github.com/deployphp/deployer/pull/1008)
 - Add support for configFile in the NativeSsh implementation [#979](https://github.com/deployphp/deployer/pull/979)
+- Add `--no-hooks` option for running commands without `before()` and `after()` [#934](https://github.com/deployphp/deployer/pull/934)


#1061 – not ~~934~~

@Elfet I don't understand this rule. I think that the link must point to an issue or PR which describes the problem. Therefore I pointed out previous PR. Isn't it correct?

I think it's better to send to actual pr with implementation. Issues if is there is commit to master, but maybe better to send to commit (or both).

@Elfet then I need to predict Pull Request's number to specify it in changelog.md? That's weird, but okay.

Kind of, or you can add more commit after pr creation. I think other contributors doing that way.

antonmedv · 2017-03-03T05:39:54Z

recipe/common.php

@@ -119,6 +119,7 @@
 option('tag', null, InputOption::VALUE_OPTIONAL, 'Tag to deploy');
 option('revision', null, InputOption::VALUE_OPTIONAL, 'Revision to deploy');
 option('branch', null, InputOption::VALUE_OPTIONAL, 'Branch to deploy');
+option('no-hooks', null, InputOption::VALUE_NONE, 'Run task without after/before hooks');


Move this to task command, as many users doesn't based on common recipe.

@Elfet Should I move only this option or all above?

Only this one. Other used by common recipe. So if your deploy.php does not require common.php, i may no need in tag/rev/branch options.

@Elfet Okay, I'll move it to TaskCommand. Meanwhile I think that it's okay to move all options to TaskCommand because of they are optional. Moreover, this way of defining options doesn't allow to add new ConsoleCommand with different arguments. I've already tried to add SSHCommand and faced that. I was not able to add required argument as there were defined options before. I guess that you will face it in version 5 refactoring.

Yes, i'm already started working on tasks args/options.

antonmedv · 2017-03-03T05:41:57Z

src/Task/Task.php

@@ -227,6 +227,10 @@ public function setPrivate()
     */
    public function addBefore($task)
    {
+        if (!is_string($task)) {
+            throw new \RuntimeException('Invalid argument to `before` hook of "' . $this->getName() . '" task.');
+        }


When it should be an InvalidArgumentException.

antonmedv · 2017-03-03T05:42:09Z

src/Task/Task.php

@@ -235,6 +239,10 @@ public function addBefore($task)
     */
    public function addAfter($task)
    {
+        if (!is_string($task)) {
+            throw new \RuntimeException('Invalid argument to `after` hook of "' . $this->getName() . '" task.');


Same here: InvalidArgumentException.

antonmedv · 2017-03-03T05:46:57Z

src/Task/ScriptManager.php

        };

-        $script = $collect($name);
+        $taskHierarchy = $collect($name);


Why are you rename this? 🙂

@Elfet I refactored it as non obvious name. $script doesn't reflect the contents as for me and $taskHierarchy does. $collect returns exactly hierarchal collection of all related tasks. If you want I will return previous name or rename to something better.

But call name is ScriptManager. What way we define a new term inside deployer. I think it is cool)

@Elfet Actually, I'd like to rename this class to TaskManager too :) It is responsible for managing tasks, not scripts. It isn't obvious for me as a newcomer. But of course it's up to you. If you want, I will return $script variable.

Please return script) I don't like naming for everything manager-it, manager-that. All what we write some sort of managers.

Updated. Hmm, it's already called manager so I've just suggested to rename to another manager :) Meanwhile, it's returned to previous state.

mkuznetsov4xxi added 2 commits March 3, 2017 04:50

Add --no-hooks option for running commands without before() and `…

41e4f2d

…after()` (deployphp#934)

Fix tests and StyleCI for --no-hooks option

70ba131

pluseg mentioned this pull request Mar 3, 2017

Added --hook option #934

Closed

pluseg commented Mar 3, 2017

View reviewed changes

antonmedv requested changes Mar 3, 2017

View reviewed changes

mkuznetsov4xxi added 2 commits March 4, 2017 00:04

Refactor --no-hooks option and all related code

0353133

Updated changelog for --no-hooks options

bef2789

antonmedv approved these changes Mar 5, 2017

View reviewed changes

antonmedv merged commit ed4957a into deployphp:master Mar 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--no-hooks` option for running commands without `before()` and `after()` (#934) #1061

Add `--no-hooks` option for running commands without `before()` and `after()` (#934) #1061

pluseg commented Mar 3, 2017

pluseg Mar 3, 2017

pluseg Mar 3, 2017

antonmedv Mar 3, 2017

antonmedv Mar 3, 2017

pluseg Mar 3, 2017

antonmedv Mar 3, 2017 •

edited

Loading

pluseg Mar 3, 2017

antonmedv Mar 3, 2017

antonmedv Mar 3, 2017

pluseg Mar 3, 2017 •

edited

Loading

antonmedv Mar 3, 2017

pluseg Mar 3, 2017

antonmedv Mar 3, 2017

antonmedv Mar 3, 2017

antonmedv Mar 3, 2017

antonmedv Mar 3, 2017

pluseg Mar 3, 2017

antonmedv Mar 3, 2017

pluseg Mar 3, 2017

antonmedv Mar 3, 2017

pluseg Mar 3, 2017

Add --no-hooks option for running commands without before() and after() (#934) #1061

Add --no-hooks option for running commands without before() and after() (#934) #1061

Conversation

pluseg commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonmedv Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pluseg Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `--no-hooks` option for running commands without `before()` and `after()` (#934) #1061

Add `--no-hooks` option for running commands without `before()` and `after()` (#934) #1061

antonmedv Mar 3, 2017 •

edited

Loading

pluseg Mar 3, 2017 •

edited

Loading