Proposal to move code from the bundle controller to separate roles.

[sndpl]

Here is my small and simple proposal to move the setup function to separate roles. Each ansible role should have it's own php class to setup the correct vars that are needed for this role. This way the bundle controller will become tiny again :)

PS: didn't fix the tests yet, since it was just a PR to discuss :)

[sndpl]

This probably needs to move to the application.php or so (never worked before with silex). Normally would do this with DI, but it's easy to refactor :)

[faabiosr]

Well, this proposal sounds like good, the ideia of maintain all roles separated is awesome, I think like you.

We have an issue #31 for this refactoring.

[InFog]

@sndpl That is what I want to do in #31. Still I have no good stuff to show you guys yet.
I think you are on the right path, so please take a look at #31 and we can work on something to fix this issue and make our code easier to maintain and, of course, easier to add new features, since this was my idea for #31.

[sndpl]

@InFog Just read your proposal in #31 and it looks like what I have :) I will try to improve it a little bit more tonight with the suggestions from that ticket.

[InFog]

Yay, @sndpl! Go for it 😄 👍

[erikaheidi]

Hey! :)

This looks like a good idea, I just want to give a heads up cause I'm currently working (as you saw) on a config-file implementation that would create a separate API for phansible. We would have a phansible-api and the website would be something completely independent. That's the idea that everybody agreed on #86 :)

How the config files will impact on this refactoring? You won't be dealing with the Renderers anymore. The controller will still deal with the form and do most part of the interpretation that happens now, but instead of creating the bundle, it will generate a config file that will be given to the API. The API will give back the bundle.

So, in the future, we will be able to have a cli that will download the bundle based on a config file.

I would say go for it, keeping in mind that the Renderers will not be used anymore in the controller. This should not affect, however, the Role refactoring.

[sndpl]

hi,

did some reading about silex and just updated the proposal:

• Removed debug config (wasn't used) and added an index_dev.php (remember to remove in your deployment scripts)
• Added service provider for roles
• Added error handler (for some reason the default exception handler of flint isn't triggered when throwing exceptions, so if somebody has an idea??) and two error templates (404 and general error)
• Added check for the /doc/{doc} urls
• Updated the urls in the templates for using the index_dev.php

@InFog / @erikaheidi does this match with your idea's written in #31 ? if not what should be changed?

[erikaheidi]

Great work. I didn't test it locally, just looked at the code, but from what I saw it's looking really good! I will test everything asap, would like to ask if other people could also test it, since it introduces major changes, that would be great.

[sndpl]

nice! Currently busy with moving the rest to roles to clean the bundlecontroller even more :) it will become a large PR, but after this it will be really easy to extend phansible with new roles.

[naxhh]

Next two days I'm on holidays so I'll try to check this in deep and provide some feedback :)

[sndpl]

@naxhh thanks! I have some major commits here waiting to be pushed, but first have to fix the ansible roles. I changed all variable names in the forms, but i think you will like it. Adding new features is now so easy with the new roles (just provide a role with initial values, a form template and an ansible role :))

current todo list:

• Fix ansible roles
• Fix layout (semantic ui doesn't seem to support multiple tab bars on a page, anybody ?? can't find any docs about their javascripts)
• Add some unit test for the new classes
• test the vagrant scripts :)

[sndpl]

It's generating a valid bundle again :) so please do some testing and let me know if you find a bug.
Current todo list:

• Fix unit tests and add some new ones
• testing, testing, testing

[sndpl]

I think this proposal is ready, but since it has become some kind of a rewrite it would be nice if people want to do some testing / code reviews.

[naxhh]

Nice. Between today and monday I'll check it all :)

Can you rebase with master in order to resolve conflicts?

[erikaheidi]

A rebase would be great.

I feel like this should be in a new separate branch, 1.0, where we could work in collaboration to come up with a solid release already using the config files and API. We could merge it sooner, and work together on tests and everything else. If this is going on master now, I'll keep using the latest tagged version for the deploys, cause it won't be safe to go live yet.

What ya'll think?

[sndpl]

I would create a 1.0 branch of the current master and use that for urgent bug fixes and use master for development (that's how i work with all my projects).

About the rebase, that will be difficult :) I checked the latest changes and updated the ones that can't be merged anymore (because code has been moved etc). This is also the reason this branch can't be too long open and we should only merge urgent bugfixes to master / 1.0 branch (depending on what we choose).

[naxhh]

another branch is a good idea.
But that will only work for bugfixes as @sndpl says.
Because keeping two branches is two times of development and test for each feature.

I like the idea of setting a branch for the current version and continue development in master for v1

[erikaheidi]

👍 I created a branch 0.6 that we should use for urgent fixes. Let's use master for dev :) 1.0 will kick ass!

I will have a look on the changes now.

[erikaheidi]

Ok, I merged it locally and I have some observations:

• What's the status of HHVM installation?
• The app role should always be the last one to be included, it's being included first
• Why do we need a vagrant_local role?
• What's the purpose of all that variables "install: '1'" in the all.yml var file?

I have the impression that you are mixing a little bit the provisioning / Ansible part with the application part that generates the provisioning, there are variables that should be part of the application only in my opinion, they don't need to be in the provisioning.

We also need to figure out better ways to present the options in the UI, the change in the webservers part from buttons to tabs, + separation of web server / PHP, made it way more complex.

Another thing: NGINX should be always the default, not Apache.

I think we can merge and work together for improving the UI and tests. I believe it's good to merge already.

Great work 👍

[sndpl]

Some answers:

• What's the status of HHVM installation?
  It's working as far as I know (with nginx, apache 2.2 and 2.4). It also makes a symlink for /usr/bin/hhvm to /usr/bin/php so you can execute cli scripts.
• The app role should always be the last one to be included, it's being included first
  Fixed.
• Why do we need a vagrant_local role?
  to support other deploy targets, the vagrant local is generating a bundle for virtualbox (and can be extended to support other like vmware too). It's now also possible for example to create a vagrant_digitalocean role, which can deploy to a droplet ;)
• What's the purpose of all that variables "install: '1'" in the all.yml var file?
  2 reasons, one to use in the ansible roles (easy to check if an role is enabled) and second, the vars.yml file is now a kind of transformed request. This way it would be possible in the future to upload your vars file and repopulate the form. That's also the reason that some vars are in the vars file that aren't used by the ansible roles.
• Nginx should be default
  Fixed.
• About the UI
  I Agree, but would like to do this in a separate ticket.

[erikaheidi]

👍 does anyone has something to say about it, any comments / suggestions / feedback? Or are we good to merge?

[naxhh]

role var is not defined

[naxhh]

i see minor improvements, but I think we can move on when the missing var is defined.

We will have time to improve things.

I didn't check any test as all agree to check them later.
I'll try to make an effort to work on that.

I'm a bit worried with some parts of the app, I see too much coupling in some places.
About the "install" part maybe is better than we create a different file for phansible, and you drag that file in the ui. not the playbook. This way we don't couple our web with the user playbook.

But as I said we can start working from this.

For me is a really nice job, so thanks a lot for this!

[sndpl]

@naxhh Thanks, just fixed the missing role :) and for the coupling, you are right, but I had to draw the line somewhere for this PR :)

About the install file, I think you are right so when this PR has been merged we should create a ticket for it and discuss how we want to do this. We are gonna need this for the API so @erikaheidi will also have some thoughts about this.

[mikeSimonson]

I made a little fix for the default value of the timezone that wasn't working anymore but for the rest it looks great.

[sndpl]

@mikeSimonson thanks! merged your fixes.

[erikaheidi]

Yes, we will have to discuss further about the "install" file, and the vars files, how we want to organize the Ansible provisioning, cause we need to give the user something that doesn't look completely different from a regular Ansible provisioning following best practices and all.

I'm gonna pull the latest changes here to merge!

[erikaheidi]

Merged! Thanks @sndpl for the big refactoring, and lets everybody get the ball kicking to improve what still could be improved, write tests and figure out the best ways for the final bundle organization (following best practices from Ansible).

👯 🎉