Hi Gil, good feedback, inline ...
On Mon, Jul 24, 2017 at 10:17 AM, gil portenseigne
<gil.portensei...@nereide.fr> wrote:
> Hello Taher,
>
> My thoughts inline :)
>
>
> On 20/07/2017 09:05, Taher Alkhateeb wrote:
>>
>> Hello Gil,
>>
>> Thank you for working on this. I have reviewed the patch and I have a few
>> notes:
>>
>> First for gradle, I don't think you need to fetch environment
>> variables. You can do whatever you want by passing flags, which might
>> be a simpler solution. So you create your own script and modify gradle
>> for your environment. You can modify the project with -P flags and the
>> JVM with -D flags
>
> We did the dev into a 13.07, so without gradle. I just re-did in gradle what
> we have done with ant with our needs.
> I agree we should use -D flags instead of env variable :).
>>
>> Now with respect to the rest of the work, I'm not sure but I think
>> this makes the code a little more fragile or brittle. Essentially you
>> are injecting multiple code locations with variables that you fetch
>> from the environment.
>
> Yes there are multi location modifications since we wanted to configure not
> only classic property files, but entityengine.xml, serviceengine.xml,
> web.xml, etc.
> I do not measure the risk of such modifications, that you're talking about,
> since default value are available.
> But we indeed feel the need to check what configuration is available within
> OFBiz to ensure its validity (hosting company job)... that is another kind
> of fragility.
>>
>> This means you are putting "state" inside application logic. One of
>> thepain points for me when refactoring OFBiz was that I change some
>> code somewhere and I get something going in flames in a completely
>> different location. Why does that happen? Because of this sort of
>> _secret_ shared state.
>
> I understand your feeling and opinion, but i am not sure that these small
> modifications are going to create flames in the app :).
Of course not :) it was an exaggeration (and my crappy attempt at some
humor). But you know what I mean right? We slowly introduce moving
parts (state) to the code which after years pile up to become a
monster. Most of my refactoring work lately is targeting removing
"state". It's very small things like this that keep getting bigger and
bigger.
> I cannot yet visualize the "state" you mentioned, now it's just a
> improvement of the way to get properties value, that should be use only in
> very controlled production environment.
> I will re-check if there is a way to minimize the several modification
> locations.
>
So allow my to explain what my interpretation of the word state. State
is essentially anything that is persisted in memory. An object is
state, a class instance field is state, an environment variable is
state, a thread is state, a process is state, a global variable is
state, and so on.
I am very much afraid of state and I try to minimize it in any code
base. Why do I do that? Well, if you have a variable (configuration in
this case) that might take multiple shapes, and you don't know at
runtime which shape would it be, then you've added a moving part. So
calling the same piece of code will not give you the same results
every time. This means code that is harder to understand, harder to
test (you need to do prep work) and harder to refactor. On the other
hand, if you do not touch the code at all, and instead do your
configuration preparation outside (using the build system for example
to generate configurations) then you avoid a whole set of problems
that come with this added logic.
>> Why not consider a different approach like either template generation
>> or multiple files. For example, instead of fetching variables and
>> introducing a new notation in the source files, instead you can have
>> test.general.properties, prod.general.properties,
>> develop.general.properties and so on. Alternatively, you can add a
>> template using freemarker, and make a new gradle task (e.g.
>> generateConfigurationFiles).
>>
>> So for example, you can have something like this:
>> ./gradlew generateConfigurationFiles -Penvironment=test
>> ./gradlew generateConfigurationFiles -Penvironment=production
>> ./gradlew generateConfigurationFiles -Penvironment=development
>> ./gradlew generateConfigurationFiles -PsourceTemplate=mySourceTemplate.ftl
>> ./gradlew generateConfigurationFiles
>> -PoverrideProperties=myOverride.properties
>>
>> Something like this would give you the ability to customize without
>> adding more moving parts. Wouldn't that achieve the same results
>> you're seeking? It might also have the advantage of maintaining
>> existing notation instead of adding this ${env: ...} notation?
>
> The original needs was to avoid modifying files in production (via
> patches/addons), to let hosting company sys admin to manage the
> configurations changes within the environment without touching code base.
> With your proposal, first we have to explain to them how to apply specific
> configuration using gradle (not a big deal), then my concern is that these
> template will have to be maintained by the sys admin, i.e. when we are
> adding some config in a property file and this one is already templated,
> communication is needed to explain what template should be adapted etc.
Okay, let's say we can have two types of templates. Ones that are
generic enough, and flexible enough, and can be maintained by us (the
community) because of their generic nature. For example
(production.properties, test.properties, develop.properties). Then we
have another type of templates that are very specific to your
environment. In that case, yes you need to maintain them yourself, but
your life would be much easier using a build system right? In fact we
can create generic tasks to generate specific templates as well.
>
> With our proposal, no communication with the hosting company is needed, but
> only when new environment variable is needed, and, no code base modification
> is done on the VM after delivery.
I'm not sure, but most modern deployments nowadays are fully
automated. You don't patch, or SSH to server to fix something. The
whole thing is managed by code. You commit something, the CI server
runs the tests and then automatically deploys to the production
environment a whole new fresh instance with the database and
everything. Communicating with the hosting company means this is done
very manually. Doesn't your hosting company have a set of web APIs to
control your VM instances?
>
> I like the template proposal (currently we use git branches to manage this
> need), this could be complementary with the "environment notation".
If you need any help with that just point me to where I might be of help.
>
> Thanks for your feedback !
Thank you for taking on this initiative :) I look forward to getting it done.
>
> Gil
>
>> WDYT
>>
>> On Tue, Jul 18, 2017 at 11:11 PM, gil portenseigne
>> <gil.portensei...@nereide.fr> wrote:
>>>
>>> Hello Guys,
>>>
>>> I just createdhttps://issues.apache.org/jira/browse/OFBIZ-9498
>>>
>>> You can test it out with the provided patch.
>>>
>>> And James, I just tested two launch to instances with different
>>> environment
>>> properties with no issue (different JVM args and database connections)
>>>
>>> Cheers
>>>
>>> Gil
>>>
>>>
>>> On 06/07/2017 22:07, gil portenseigne wrote:
>>>>
>>>> Hi James,
>>>>
>>>> inline
>>>>
>>>> On 06/07/2017 12:12, James Yong wrote:
>>>>>
>>>>> Hi Gil,
>>>>>
>>>>> Some afterthought questions with the proposed change:
>>>>> 1) Can it support multiple OFBiz instances on the same server?
>>>>
>>>> Using different shell process to launch the multiple OFBiz instances, i
>>>> think that it's possible to set environment variable dedicated to each
>>>> OFBiz
>>>> instance.
>>>>>
>>>>> 2) Can we store encrypted values for some of the environment variables
>>>>> like database credentials?
>>>>
>>>> You can store whatever value you want into environment variable, but you
>>>> must adapt OFBiz to decrypt the value.
>>>>
>>>> Thanks all for your feedback, i'll work on trunk to make it available
>>>> soon
>>>> (within a new Jira)
>>>>
>>>> Regards
>>>>
>>>> Gil
>>>>
>
