✨ ❄️ 💡 ⬆️ Update to Homestead ^5 and use per project & make command for config #27

svpernova09 · 2017-06-20T13:51:39Z

This utilizes the Homestead features where you can specify a default Homestead.yaml.example file that is used to build the actual Homestead.yaml file when a user runs ./vendor/bin/homestead make. This is essentially a dist type file. Also adds aliases and after.sh files from Homestead to further customize and add some handy aliases into the VM.

…or config This utilizes the Homestead features where you can specify a default `Homestead.yaml.example` file that is used to build the actual `Homestead.yaml` file when a user runs `./vendor/bin/homestead make`. This is essentially a dist type file. Also adds `aliases` and `after.sh` files from Homestead to further customize and add some handy aliases into the VM.

driesvints · 2017-06-25T19:26:29Z

Homestead.yaml.example

+databases:
+    - homestead
+name: pastebin
+hostname: pastebin


Good idea to keep it as an example file 👍

driesvints

Thanks for your work on this ❤️

Some of these are questions and some things I'd like to revert. Take your time to review them and get back to me. Feel free to ask questions/give feedback.

driesvints · 2017-06-25T19:26:51Z

after.sh

+
+# If you would like to do some extra provisioning you may
+# add any commands you wish to this file and they will
+# be run after the Homestead machine is provisioned.


Since we're not adding anything to this file, can't we just remove it?

We certainly can. If we need it in the future we can just re-run the Homestead make command or just simply drop this file into the root and Homestead will pick it up and execute it automatically. Will remove it for now.

driesvints · 2017-06-25T19:28:07Z

aliases

+        "$@"
+
+    if ! $XDEBUG_ENABLED; then xoff; fi
+}


I think it's best to not include this file on the main repo and only allow people to add it if they want to. Or perhaps add it in Homestead itself? In any case, it has little to do with the Pastebin source code...

The aliases file is a lot of support functions for generating host files for nginx as well as a ton of helper methods for toggling xdebug on and off. Honestly we can remove whatever you want to pear down the file to only what the specific project needs. I would recommend leaving the command shortcuts so that anyone used to having those in a Homestead project would still find them in a project that's using Homestead.

Would it be an idea to make the Homestead.yaml file to load in the aliases based on a filepath so the aliases file doesn't needs to be committed?

I'm sorry but I'm not really a fan of committing these sorts of files to a project's codebase. They seem to be too personal. I doubt I'll ever make use of most of them.

We can gitignore the aliases file if you prefer. That way when you run the make command they will be there if you want them, but won't be checked in.

Yes, that's a good idea 👍

driesvints · 2017-06-25T19:29:23Z

Homestead.yaml.example

+keys:
+    - ~/.ssh/id_rsa
+folders:
+    - { map: ~/PhpstormProjects/pastebin, to: /home/vagrant/pastebin }


Can you change ~/PhpstormProjects/pastebin back to ~/Sites/pastebin?

driesvints · 2017-06-25T19:30:06Z

composer.json

@@ -15,7 +15,7 @@
    "require-dev": {
        "codeclimate/php-test-reporter": "^0.4.4",
        "fzaninotto/faker": "^1.6",
-        "laravel/homestead": "^5.0",
+        "laravel/homestead": "^5",


Any particularly reason why you changed this?

Homestead follows semantic versioning and we end up doing minor releases somewhat often. It's easy to fall behind if you're sticking to a major version. With this change we'll get 5.* instead of only 5.0.*

driesvints · 2017-06-25T19:30:58Z

readme.md

@@ -28,35 +28,19 @@ Please make sure you install the following tools before starting with the instal

 ## Installation

-> Note that you're free to adjust the `~/Sites/pastebin` location to any directory you want on your machine.
+> Note that you're free to adjust the `~/Code/pastebin` location that will be in the `Homestead.yaml` to any directory you want on your machine.


Can you change Code back to Sites?

This section needs to be re-worded.

The Homestead make command will automatically figure out where it is on the filesystem and map the current project to the full filesystem path. This means the folder mapping that you see in the Homestead.yaml.example:

folders: - { map: ~/Sites/pastebin, to: /home/vagrant/pastebin }

Will get transformed to:

folders: - map: /Users/halo/PhpstormProjects/pastebin to: /home/vagrant/Code/pastebin

(For me because /Users/halo/PhpstormProjects/ is where I keep my projects, the path you will see is unique to that computer)

driesvints · 2017-06-25T19:32:47Z

readme.md

 3. Run `vagrant up`
-4. SSH into your Vagrant box, go to `/home/vagrant/pastebin` and run the following commands:
+4. SSH into your Vagrant box, go to `/home/vagrant/Code/pastebin` and run the following commands:


Can you revert this to make it the same as in the Homestead.yaml.example file?

With the current release of Homestead they will see /home/vagrant/Code/pastebin mapping in the VM (unless they changed that in their Homestead.yaml). I have removed the /Code/ part of the path in laravel/homestead#601 but I haven't yet tagged a new release. So we should leave this for now IMHO.

driesvints · 2017-06-25T19:35:15Z

readme.md

-```
-
-You can now visit the app in your browser by visiting [http://pastebin.app/](http://pastebin.app).
+    2. `cp .env.example .env`


This is unnecessary as it's done with a Composer hook. Check the composer.json file.

I've checked this twice now, cloning the repo to a new folder and running composer install does not trigger the post-root-package-install. no .env file is created, and post-create-project-cmd must not be getting run either because I don't see an error that it's complaining about a missing .env

I believe we may be using the wrong event names. From https://getcomposer.org/doc/articles/scripts.md

This section:

"post-create-project-cmd": [ "php artisan key:generate" ],

occurs after the create-project command has been executed. Which we're not using the create-project command so it will never fire.

This section:

"post-root-package-install": [ "php -r \"file_exists('.env') || copy('.env.example', '.env');\"" ],

occurs after the root package has been installed, during the create-project command.*

Which also never fires based on our install instructions. I believe we should remove those 2 sections and update post-install-cmd to this:

"post-install-cmd": [ "php -r \"file_exists('.env') || copy('.env.example', '.env');\"", "php artisan key:generate", "Illuminate\\Foundation\\ComposerScripts::postInstall", "php artisan optimize" ],

This seems to work well.

Trailing output of composer install

Generating autoload files > php -r "file_exists('.env') || copy('.env.example', '.env');" > php artisan key:generate Application key [base64:6wmjpDjau3QMIpR1L0MTRXzMPcaos08yr618pCTikNI=] set successfully. > Illuminate\Foundation\ComposerScripts::postInstall > php artisan optimize Generating optimized class loader The compiled services file has been removed.

driesvints · 2017-06-25T19:38:08Z

readme.md

+    4. `php artisan migrate`
+5. Add `192.168.10.30 pastebin.app` to your computer's `/etc/hosts` file. (optional)
+
+You can now visit the app in your browser by visiting [http://pastebin.app/](http://pastebin.app) or [http://localhost:8000/](http://localhost:8000) if you prefer to not edit your hosts file.


I'd like to keep this part simple tbh. Can you revert this and remove the optional from the last step? I know it's indeed not necessary but I'd like everyone to use the same install instructions. Everyone is still free to set up the app as they like but for the sake of having everyone on the same workflow I'd rather have one way to set the app up detailed in the install instructions.

I've simplified it to

## Installation > Note that you're free to adjust the `~/Code/pastebin` location that will be in the `Homestead.yaml` to any directory you want on your machine. 1. Clone this repository: `git clone git@github.com:laravelio/pastebin.git pastebin` 2. Run `./vendor/bin/homestead make --no-after` 3. Run `vagrant up` 4. SSH into your Vagrant box, go to `/home/vagrant/Code/pastebin` and run the following commands: 1. `composer install` 3. `php artisan migrate` 5. Add `192.168.10.30 pastebin.app` to your computer's `/etc/hosts` file. You can now visit the app in your browser by visiting [http://pastebin.app/](http://pastebin.app).

Based on previous comments and replies

driesvints · 2017-06-25T19:46:54Z

readme.md


 1. Clone this repository: `git clone git@github.com:laravelio/pastebin.git pastebin`
-2. Add the `Homestead.yaml` file from below to the root of your project
+2. Run `./vendor/bin/homestead make`


I've read the help details for this command but from my understanding it needs the --example flag in order to copy the Homestead.yaml.example file, correct? What will this command do besides that?

The example flag is to generate a sample Homestead.yaml.example file that a user can review and/or compare to the Homestead.yaml they're editing. By default the make command will use the example file as a template if it exists.

What we do need to edit here is add the no after flag so the user doesn't create the after.sh file (it's created by default)

driesvints · 2017-06-25T19:48:33Z

readme.md

@@ -28,35 +28,19 @@ Please make sure you install the following tools before starting with the instal

 ## Installation

-> Note that you're free to adjust the `~/Sites/pastebin` location to any directory you want on your machine.
+> Note that you're free to adjust the `~/Code/pastebin` location that will be in the `Homestead.yaml` to any directory you want on your machine.


Can you move this comment inside the Homestead.yaml.example file so it's on top and revert your changes to this sentence?

I believe the readme.md needs to be updated to explain what I described #27 (comment) We can add comments to the example file however at the moment they won't be persisted into the Homestead.yaml file. I've opened a Homestead issue to look into this: laravel/homestead#602

Ok let's keep it here for now. It's just a bit weird since there's no reference to it anymore in the readme.

driesvints · 2017-06-25T19:50:19Z

Can you also rebase with master and fix the merge conflict?

… scripts

svpernova09 · 2017-06-26T23:44:04Z

Hopefully all that made sense, if not I'm happy to jump on a google hangout or something to review.

driesvints

A bit more feedback and some remarks & questions.

driesvints · 2017-06-30T21:15:32Z

aliases

+        "$@"
+
+    if ! $XDEBUG_ENABLED; then xoff; fi
+}


Would it be an idea to make the Homestead.yaml file to load in the aliases based on a filepath so the aliases file doesn't needs to be committed?

I'm sorry but I'm not really a fan of committing these sorts of files to a project's codebase. They seem to be too personal. I doubt I'll ever make use of most of them.

driesvints · 2017-06-30T21:17:37Z

composer.json

        "post-install-cmd": [
+            "php -r \"file_exists('.env') || copy('.env.example', '.env');\"",
+            "php artisan key:generate",


Wouldn't this re-generate the key every time you install your dependencies? I think this might break production in that case and clear all sessions. I think it's best that we leave this as Laravel's default template.

That's a good point, it would regenerate the key on every composer install however we should then add that command to the setup instructions since it wouldn't be run on install.

Ok you can add it to the install instructions then 👍

driesvints · 2017-06-30T21:19:25Z

readme.md

@@ -28,35 +28,19 @@ Please make sure you install the following tools before starting with the instal

 ## Installation

-> Note that you're free to adjust the `~/Sites/pastebin` location to any directory you want on your machine.
+> Note that you're free to adjust the `~/Code/pastebin` location that will be in the `Homestead.yaml` to any directory you want on your machine.


Ok let's keep it here for now. It's just a bit weird since there's no reference to it anymore in the readme.

driesvints · 2017-06-30T21:19:58Z

readme.md

-name: pastebin
-hostname: pastebin
-```
+    3. `php artisan migrate`


Change this back to 2

driesvints · 2017-06-30T21:20:19Z

readme.md

-hostname: pastebin
-```
+    3. `php artisan migrate`
+5. Add `192.168.10.30 pastebin.app` to your computer's `/etc/hosts` file.


Remove the punctuation here

driesvints · 2017-06-30T21:24:38Z

Homestead.yaml.example

+keys:
+    - ~/.ssh/id_rsa
+folders:
+    - { map: ~/Sites/pastebin, to: /home/vagrant/pastebin }


If /home/vagrant/Code/pastebin is going to be the default path after make, it might be best to immediately reflect that here. Sorry, didn't realize this before.

On a side note: I think it's pretty weird for per-project Homestead installations to make use of a Code subdirectory. It makes sense for the global Homestead installation but I wouldn't use it on a per-project install.

I agree 100% on the Code folder, which is why it's been removed on master, just haven't tagged a new version yet.

Ah cool! Thanks for that :)

… Code in example

…e in readme

svpernova09 · 2017-07-01T16:38:12Z

That should be everything so far. I did add the Code folder back to the example just for the sake of clarity on what an end user would see right this second. I'll update this later once I tag a new Homestead version.

driesvints · 2017-07-01T16:45:29Z

@svpernova09 you'll have to rebase the branch before I can merge.

svpernova09 · 2017-07-01T16:49:33Z

What's the need for the rebase? I don't see any conflicts. The 5 commits can be automatically squashed via "merge and squash" option if that's the issue.

driesvints · 2017-07-01T16:53:04Z

@svpernova09 I'm not sure where the conflicts are coming from. I usually rebase new commits on top of the base branch with the "Rebase and merge" option:

svpernova09 · 2017-07-01T17:03:31Z

No idea what I've done but I've totally broken my local branch, will re-submit as a fresh PR

driesvints reviewed Jun 25, 2017

View reviewed changes

driesvints requested changes Jun 25, 2017

View reviewed changes

svpernova09 added 2 commits June 26, 2017 08:16

⬆️ 🔥 Resolve composer.lock conflicts, remove unused after.sh file

6d1d78b

✨ ❄️ 💡 📚 Update install notes, revert paths to Sites, Update composer…

2888d8a

… scripts

driesvints requested changes Jun 30, 2017

View reviewed changes

svpernova09 added 2 commits July 1, 2017 11:33

❄️ 🎀 Tweak readme based on feedback, ignore after.sh and aliases, add…

66a4123

… Code in example

❄️ 🎀 Remove key:generate from post install script, update key:generat…

d76a973

…e in readme

driesvints approved these changes Jul 1, 2017

View reviewed changes

svpernova09 closed this Jul 1, 2017

svpernova09 mentioned this pull request Jul 1, 2017

✨ ❄️ 🎀 Use Homestead per project install, updated readme, add example config #31

Merged

svpernova09 deleted the feature/homestead-per-project branch July 1, 2017 17:13

✨ ❄️ 💡 ⬆️ Update to Homestead ^5 and use per project & make command for config #27

✨ ❄️ 💡 ⬆️ Update to Homestead ^5 and use per project & make command for config #27

Conversation

svpernova09 commented Jun 20, 2017

Choose a reason for hiding this comment

driesvints left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driesvints commented Jun 25, 2017

svpernova09 commented Jun 26, 2017

driesvints left a 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

svpernova09 commented Jul 1, 2017

driesvints commented Jul 1, 2017

svpernova09 commented Jul 1, 2017

driesvints commented Jul 1, 2017

svpernova09 commented Jul 1, 2017