cf-charmers team mailing list archive

[Cory Johns <cory.johns@xxxxxxxxxxxxx>]

Alex L.,

Ben did indeed tell me about lbox, and I have gotten it set up and will be
creating merge proposals for the changes I made to charm-helpers and the
respective charms.

To address your specific comments:

* There's no reason we couldn't have a separate sub-folders under
templates, whether consists of templates/upstart and templates/config, or
templates/service1, templates/service2, etc;  I considered it, when moving
the files, and I didn't think there were enough files to warrant it, but I
was on the fence and can easily be swayed toward better organization

* While many upstart conf files don't need any interpolated values, some do
(or at least some tasks could be cleaner if a common base was used, with
values changed out for specific services).

* However, even if upstart conf files never have interpolated values,
folding install_upstart_jobs into services.register should reduce
complexity, since there will be a single, explicit configuration block that
lists all of the files associated with a given service, with all service
information in a single, declarative location. I was certainly surprised at
the implicit association between files not mentioned anywhere in the hooks
and the declarative services block.  Eventually, the declarative services
block could be all that most charms need (and possibly even broken out into
a services.yaml file), and the boilerplate hooks code could be refactored
into charm-helpers.

* My understanding of the services.register() block was that it described
the services managed by the charm, in which case it makes sense to
explicitly list the upstart conf along with the config files associated
with the service, and any context that each file might need to be complete.

* Skipping "re-rendering" is an optimization that would be easy enough to
add; it's trivial to detect when a "template" requires no context and skip
the jinja2 pipeline and potentially skip re-copying as well.  That, plus
handling of binary assets / files required by a service (which would
actually just be part of the skipping jinja2 pipline optimization) were
improvements I hoped to address in the near future.


I hope that makes my thought processes on these changes more clear.
 Obviously, these changes are still being finalized and this is the point
to work out how to best make a framework that can support, not just the
Cloud Foundry Charms, but as many Charms in the general ecosystem in the
best way possible (since I very much came at this from the point of view of
refactoring clearly useful functionality into the core for general use).  I
feel that a declarative system that lets you say, "These are my services,
these are the files that need to be populated for those services to
function, this is the contextual data those files need, and this is where
they go," will be a very powerful, flexible way to write Charms.

I look forward to productive discussion regarding these proposed refactors.

- Cory


On Thu, May 15, 2014 at 6:12 AM, Alexander Lomov <lomov.as@xxxxxxxxx> wrote:

> Hi, all.
>
> Cory, we agreed to use Merge Proposals to update trunk. I have used lbox
> [1] to merge changes. You can ask Ben about details.
>
> Ben, could you review changes in 32nd revision in cf-dea charm [2]? I'm
> not sure that it is good idea to move upstart job files to template folder
> because :
>
> - first of all it brings mess to templates folder (after this refactor
> there is no any structure there and config file are on the same level with
> upstart jobs)
> - upstart jobs files are not templates (and I think it's better to leave
> them as they are in current version of charms)
> - using `services.register` for upstart jobs against
> `install_upstart_jobs` increases amount of code
> - using files that are not templates in `services.register` declaration is
> not idea behind services module of charm helpers (we always used installed
> file that are not templates within install hook [3, 4])
> - upstart jobs will be rerendered every time we start service; it's not so
> bad, but I prefer to avoid unnecessary actions.
>
>
> Thank you,
> Alex L.
>
>
> References:
> [1] http://blog.labix.org/2011/11/17/launchpad-rietveld-happycodereviews
> [2]
> http://bazaar.launchpad.net/~cf-charmers/charms/trusty/cf-dea/trunk/revision/32
>
> On 14 May 2014 22:20, Cory Johns <cory.johns@xxxxxxxxxxxxx> wrote:
>
>> Alex,
>>
>> My apologies.  That was supposed to go to a feature branch, but was
>> applied to a lightweight checkout by mistake and so automatically pushed
>> itself.  I was told, though, that it wouldn't be a problem, since the
>> bundle was tagged to specific revisions of the charms, but then the tagging
>> was removed in trunk after that, which caused it to become an issue.
>>
>> So, I've pushed a revert commit to fix cf-dea/trunk, and the feature
>> branches are up for review separately and can be all merged together when
>> ready, as they were intended.
>>
>>
>> On Wed, May 14, 2014 at 12:22 PM, Prismakov Alexander <
>> alexander.prismakov@xxxxxxxxxxx> wrote:
>>
>>>  Hi Cory,
>>>
>>>  After 32 revision of cf-dea we have broken upstart jobs (e.g.
>>> cf-warden actually contains dir-server job)
>>> Please take look here -
>>> http://bazaar.launchpad.net/~cf-charmers/charms/trusty/cf-dea/trunk/view/head:/hooks/hooks.py#L48<http://bazaar.launchpad.net/~cf-charmers/charms/trusty/cf-dea/trunk/view/head:/hooks/hooks.py#L52>
>>>
>>>  Guys, please be more careful with pushing code to trunk. Code need to
>>> be checked by integration tests (at least manually)
>>> before applying to trunk.
>>>
>>>  Now we need to redeploy CF, reconfigure and rerun MySQL broker etc.
>>>
>>>  -Alex Prismakov
>>>
>>>
>>>
>>>
>>
>> --
>> Mailing list: https://launchpad.net/~cf-charmers
>> Post to     : cf-charmers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~cf-charmers
>> More help   : https://help.launchpad.net/ListHelp
>>
>>
>