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

Optimize command updated #11

Merged
merged 7 commits into from
May 3, 2018
Merged

Conversation

BadChoice
Copy link
Contributor

@BadChoice BadChoice commented Apr 26, 2018

In new laravel version the optimize command is not necessary anymore, but you can use it in the composer install. I also added the --prefer-dist flag as it will download the dist files if available instead of all the sources and it makes it faster

I also added the envoy run rollback that will switch to the previous one in case something is messed up

Jordi Puigdellívol and others added 3 commits April 26, 2018 09:39
… but you can use it in the composer install. I also added the --prefer-dist flag as it will download the dist files if available instead of all the sources and it makes it faster
So we can make sure the page still returns a 200, and if not, do an automatic rollback

(the automatic rollback is not done yet)
@BadChoice BadChoice mentioned this pull request Apr 26, 2018
Copy link
Member

@davidrushton davidrushton left a comment

Choose a reason for hiding this comment

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

Awesome - thanks @BadChoice for this update!

A couple of quick notes that might be good to change before I merge and tag this release:

  • You've changed the date format to Y-m-d_H:i:s, but this is used for the folder name so YmdHis would be preferable (avoiding colons in folder name)

  • I understand your naming conventions, but to make it easier for watchers to upgrade, would you be able to rename: deployment_git back to deployment_start; deployment_update_current back to deployment_finish?

  • The health_check task is a great idea. It's included in the core deploy job by default, but I think this would be best as an option when deploying (similar to deployment_option_cleanup using @if ( isset...

Let me know your thoughts and thanks again!

David.

@BadChoice
Copy link
Contributor Author

Thanks for the review, let me explain why I did those changes so we can decide which is the best option

  • About the date, I find it hard to figure the date when there are no separators at all, I don't know if there can be any issue by having dashes or colons in the folder name? if the problems are the colons, we might use another character as delimiter so it is easy to see the date?

  • I believe it is more readable the git and update_current than start and finish, as you don't have what it internally does by seeing start and finish (I'm not really happy with the update_current name thought, maybe update_symlink_to_current would be better)

  • My idea was that every time you do a deploy, it does a health check to make sure the site is still up and running, so if not, you can easily do an envoy run rollback to go to the last deploy.
    Do you see any problem in doing the health check in the deploy?

I would also like to add in another pr the possiblity to be able to deploy to stagging and to production by doing envoy run deploy --production

Thank you!

@davidrushton
Copy link
Member

Thanks.

  • Other deployment tools seem to follow the YmdHis format for deployment folders, including envoyer.io which this is largely a do-it-yourself version of. I'd rather keep that format going forward, but changing the format is just one line of code once pulled into a Laravel project - https://github.com/papertank/envoy-deploy/blob/master/Envoy.blade.php#L18 - so it should be easy for anyone to do.

  • I appreciate your point about naming conventions of tasks, and I always prefer to keep code explanatory. On the base Envoy script in this repo however, I tried to keep names general (like start and finish, so that others could easily see where to add and edit their own tasks (restarting php, etc), and i'd rather keep it that way for easiest upgrading / tweaking.

  • I like the health check feature - thanks for the PR. As far as I can see though, it's required - i.e it will try to call the URL even if nothing is set in DEPLOY_HEALTH_CHECK. Not everyone would use or want to enable this, and i'd rather not add features that are forced. Can I suggest:

@task('health_check')
	@if ( ! empty($healthUrl) ) 
		...
	@endif
@endtask
  • I'd be interested how you suggest having staging and production deployment on the same deployment script. Envoy obviously has the feature for multiple servers, but I don't see the best way to switch between. I don't usually use zero-downtime deployments on staging since there's not any live traffic to care.

Cheers, D.

@BadChoice
Copy link
Contributor Author

Just updated the PR with the changes we talked,
I added the more detailed date format as a comment in case anybody wants to use it

Let me know if you want me to change anything else

@davidrushton davidrushton merged commit a261201 into papertank:master May 3, 2018
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.

2 participants