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

Add ['override' => true] option in task #1487

Closed
antonmedv opened this issue Jan 3, 2018 · 8 comments
Closed

Add ['override' => true] option in task #1487

antonmedv opened this issue Jan 3, 2018 · 8 comments

Comments

@antonmedv
Copy link
Member

Q A
Issue Type Feature Request
Deployer Version v7

Description

Now you can override task really simple:

task('deploy', [...]);

// override:
task('deploy', [...]);

What I'm proposing is to add explicit parameter for overriding:

task('deploy', [...]);

// override:
task('deploy', ['override' => true], [...]);

With out 'override' => true override will fail with exception:

Can't override "deploy" task. If you want to override task, add 'override' => true option:
    
    task('deploy', ['override' => true], ...);
    
@antonmedv antonmedv added this to the 7.0 milestone Jan 3, 2018
@staabm
Copy link
Contributor

staabm commented Jan 3, 2018

IMO recipes are already rather hard to handle and error prone when customizing. Allowing this level of overrides will make it even more error prone.

IMO one should copy and change the recipe instead which also results in less complexity and a more reasonable file for the user

@antonmedv
Copy link
Member Author

@staabm Disagree. Recipes really nice approach, over time Deployer recipes with help from dozen contributors got it shape, features and better error handling.
Also deployer api make is it really easy to understand what is going under hood.

This proposal about more explicit way of overriding tasks. Which already happens.

@antonmedv
Copy link
Member Author

antonmedv commented Jan 3, 2018

As alternative solution:

Leave API as is and in debug mode -vvv show warning:

warning: Task "deploy" from "deploy.php" overrides task "deploy" from "common.php".

@staabm
Copy link
Contributor

staabm commented Jan 3, 2018

Disagree. Recipes really nice approach, over time Deployer recipes with help from dozen contributors got it shape, features and better error handling.
Also deployer api make is it really easy to understand what is going under hood.

I didnt say that the recipes are bad, but only that working with them, because of lots of variables in it is DX wise a real problem.
In my opinion we should not further promote making them even more dynamic.

people shouldn't re-use only parts of the recipe. if the need 80% of them, just copy it and adjust what you need helps them in the long run and also keeps the builtin-recipes straigt forward and maintainable.

the task of writing a good deployment recipe is already rather hard and gets even more complex the more tools you have. having even more dynamic parts in it doesnt help to make it more reliable.

@skarnl
Copy link
Contributor

skarnl commented Jan 3, 2018

I think that if you are using 80% of a recipe, you shouldn't copy it whole. That way you will branch off from the original and won't benefit from the community work that is put into the recipe in the future.
I think it's better to think about a solution how to manipulate / adjust / tweak a recipe easily (to read and to maintain).

For instance, if you want to just disable one task you now need to overwrite it. How about we have some kind of easy wrapper function called disable( 'taskname' ) or remove('taskname') - so you can just use the full recipe potential, but make it also very clear you don't want to use one/two specific step.

And OT, I do think it's a good solution to explicit mark some task to be overwritten and otherwise throw an error. It's not much work to add the overwrite => true and it will prevent stupid mistakes.
Even when you don't know a task might already excists and by accident you overwrite it, now you would receive an error and the developer can explicit mark it's intentions or rename the task.

Only displaying this in the -vvv mode would be not usefull imho. That way you would only see it if you were looking for it... and if you were looking for it, you would already know you did it ^^
And in the -vvv mode there is so much information, this small warning notice would get lost and not seen.

So my vote is: yes, implement it and throw an error.

@staabm
Copy link
Contributor

staabm commented Jan 3, 2018

In my opionion you already expressed the problems I also had in #1488 and this is the reason why I think that reusing the recipe and override stuff isnt a good idea.

@antonmedv
Copy link
Member Author

@staabm actually this proposal makes overriding harder (because you need to add ['override' => true] manually). So, it's actually what you want. Note: now overriding already working. Without warrning.

@staabm
Copy link
Contributor

staabm commented Jan 4, 2018

since its already possible I am +1 for making it explicit.

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

No branches or pull requests

3 participants