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

Added --hook option #934

Closed
wants to merge 4 commits into from
Closed

Added --hook option #934

wants to merge 4 commits into from

Conversation

peterjaap
Copy link
Contributor

@peterjaap peterjaap commented Dec 23, 2016

Added --hook option to disable running other tasks than the one specified

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

I wanted a way to run certain tasks without having the before/after hooks set in deploy.php to be run, because some of those tasks might assume we are in a deploy context while maybe we're not.

We can now do;

dep custom:command:here staging --hooks false

While we have

before('custom:command:here', 'another:command:before:custom');
after('custom:command:here', 'another:command:after:custom');

So not to run another:command:after:custom or another:command:before:custom but still run custom:command:here.

@peterjaap
Copy link
Contributor Author

Tests are failing, though I don't understand why.

@antonmedv
Copy link
Member

I think test failing in parallel tests. Try fix it myself.

@antonmedv
Copy link
Member

@peterjaap also i think test for this options will be cool.

@peterjaap
Copy link
Contributor Author

The test actually revealed a bug in the code, heh, who would've thought. All checks passing!

@antonmedv
Copy link
Member

What about renaming to --no-hooks?

@peterjaap
Copy link
Contributor Author

peterjaap commented Jan 19, 2017 via email

@@ -62,6 +62,13 @@ protected function execute(Input $input, Output $output)
$stage = $input->hasArgument('stage') ? $input->getArgument('stage') : null;

$tasks = $this->deployer->getScriptManager()->getTasks($this->getName(), $stage);

if (!$input->getOption('hooks') || $input->getOption('hooks') === 'false') {
Copy link
Member

Choose a reason for hiding this comment

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

But what about group task? Them will not be working like this :/

@@ -118,6 +118,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('hooks', null, InputOption::VALUE_OPTIONAL, 'Set to false to skip after/before hooks', true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to change to no-hooks. Also please set $mode = InputOption::VALUE_NONE and remove default value. Then in TaskCommand:66 you can use just one simple condition if ($input->getOption('no-hooks')) {.

if (!$input->getOption('hooks') || $input->getOption('hooks') === 'false') {
$tasks = array_filter($tasks, function ($task) {
return $this->getName() == $task->getName();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

@peterjaap I guess you should implement ScriptManager::setHooksEnabled(true|false) and use the $isHooksEnabled flag in ScriptManager::getTasks() method. It will work both for single and group tasks.

Also nice to have tests for that. Check test/src/FunctionsTest.php with pretty same ones.

Peter, could you do that? Do you have a free time?

@pluseg
Copy link
Contributor

pluseg commented Mar 3, 2017

@Elfet I've rewritten this PR and added tests. Please close this ticket and review #1061.

@antonmedv
Copy link
Member

Closing as duplicate of #1061 (this pr fixes group scripts)

@antonmedv antonmedv closed this Mar 3, 2017
antonmedv added a commit that referenced this pull request Mar 5, 2017
Add `--no-hooks` option for running commands without `before()` and `after()` (#934)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants