factorial-io / phabalicious

Supports your deployments and every-day devops-tasks
http://docs.phab.io
MIT License
18 stars 3 forks source link

Feature/integrated multibasebox #68

Closed stmh closed 4 years ago

stmh commented 4 years ago

Hi Shibin,

(I know you are still on holidays, feel free to decline the review)

Here are some code changes to introduce two new commands:

Both commands will try to streamline the process to setup and update a local multibasebox installation. Both commands use local scaffold files (packaged into the phar), so we can introduce changes with new versions of phabalicious. The github repository for multibasebox is still used as a base.

The PR includes other changes, all tokens acquired via questions or command line options will be saved as yaml in the project-folder, (in a file called .phab-scaffold-tokens.yml) so additional invocations of a scaffold command can pick up the saved tokens and reuse them. That will enhance the DX when something fails, as you do only need to provide a name like phab app:scaffold foo --name bar

After a successful review I'll add some documentation and most importantly change the documentation on https://github.com/factorial-io/multibasebox, so it refers to phabalicious on how to install multibasebox.

In the end, multibasebox will be a feature of phabalicious.

d34dman commented 4 years ago

I didn't do code review. However did some manual testing. And here are my comments.

workspace:create I did a trial in my local. We have a situation where it can fail if the destination folder, and particularly the certs folder in destination folder is not a shared folder in Docker.

It does give a nice message on how to proceed. Which is awesome. However after adding the certs (i.e. /Users/shibindas/multibasebox/certs in my case) folder to shared folder, we can't run workspace:create again, since destination folder already exists. So I proceeded with workspace:update command.

Suggestion : Improve documentation regarding File Sharing settings in docker. Result : ✅pass

workspace:update

This is awesome. Since workspace:create failed for me the first time due to file sharing permission, this command help me complete the installation. Point to be noted is, it has to be run inside the multibasebox folder.

Result : ✅pass

Both commands use local scaffold files (packaged into the phar), so we can introduce changes with new versions of phabalicious.

I was not able to figure out, how to test this. When i did which phab returns /usr/local/bin/phab in my local, and not something that is installation specific. (I had scaffolded a project using /Users/shibindas/work/github/phabalicious/bin/phab app:scaffold https://config.factorial.io/scaffold/d8/d8.yml` command).

Result : ???

The PR includes other changes, all tokens acquired via questions or command line options will be saved as yaml in the project-folder, (in a file called .phab-scaffold-tokens.yml)

This is awesome feature :) Filename seems to be .phab-scaffold-tokens i.e. it doesn't have yml extension. Is this intentional? Result : ✅pass

stmh commented 4 years ago

thanks for testing!

Regarding the certs issue: I'll update multibasebox so it doesnt require the mapping of certs into the container, should increase the OOBE

Regarding workspace:update: in theory it should be enough to be in a subfolder of multibasebox, will test this again.

Regarding the packaged scaffold files: they are part of the phar and not visible for the user. But we can update them and distribute changes with regular phab upates.