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

Bugfix laravel recipe #622

Merged
merged 2 commits into from Apr 13, 2016
Merged

Conversation

oanhnn
Copy link
Contributor

@oanhnn oanhnn commented Apr 12, 2016

Q A
Bug fix? Yes
New feature? No
BC breaks? No
Deprecations? No
Fixed tickets #527 #572

In this PR:

  • Replace php by {{bin/php}}
  • Add helper tasks about migration and option --force can set by env()
  • Resolved issues about shared directories and .env file
  • Resolved issues about writable storage directory. Tested with setfacl on CentOS
  • Changed order task deploy:shared and deploy:vendors in laravel recipe (See Laravel Recipe Improvements #619)

@antonmedv
Copy link
Member

Does it will be working without --force at all?

@barryvdh
Copy link
Contributor

Migrate is useless without --force on production, it will never run.

What about adding options to the deploy:migrate --force command (if that's possible), or asking confirmation before running?

@barryvdh barryvdh mentioned this pull request Apr 12, 2016
6 tasks
@oanhnn
Copy link
Contributor Author

oanhnn commented Apr 12, 2016

WIth this PR, users can use --force option or not by env()

sever('prod', 'xx.xx.xx.xx', 22)
  ->env('allow_force_migrate', false);

server('dev', 'xx.xx.xx.xx', 22)
   ->env('allow_force_migrate', true);

Default env('allow_force_migrate', false);
And task deploy:migrate is not included deploy task.

@barryvdh
Copy link
Contributor

I understand, but without --force, it's just useless.

@barryvdh
Copy link
Contributor

I've listed some more ideas BTW, if you feel like adding more stuff ;)
#619

@oanhnn
Copy link
Contributor Author

oanhnn commented Apr 12, 2016

@barryvdh
Option --force is very dangerous. Users have to control it. We wont add it as default.

@oanhnn
Copy link
Contributor Author

oanhnn commented Apr 12, 2016

@Elfet Please review and merge and close issue refs

@antonmedv
Copy link
Member

If --force is stand only for non-interactive, i think it's ok to force all the time (Better DX).

@barryvdh
Copy link
Contributor

I understand, but setting the force value in your config doesn't make it less dangerous (because it's set in the deploy file, so you still run it accidentally)
Why not do askConfirmation('Are you sure you want to migrate') with an option to always force?

@antonmedv
Copy link
Member

askConfirmation it's always bad pattern in automation tools :(

@barryvdh
Copy link
Contributor

Didn't you put that in the functions.php? :P And it's also in drupal7 recipe. But fair enough.
Imho the --force flag is similar to --non-interactive. I opened and issue here to fix that: laravel/framework#13125

@oanhnn oanhnn removed the feature label Apr 13, 2016
@oanhnn
Copy link
Contributor Author

oanhnn commented Apr 13, 2016

@Elfet @barryvdh
I split this PR to:

@oanhnn oanhnn merged commit 4238fb0 into deployphp:master Apr 13, 2016
@oanhnn oanhnn added bugfix and removed bug labels May 19, 2016
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.

None yet

3 participants