Hi Christian,
Before arguing on each point, David, Guillaume, and Andreas sent a -1.
So please, revert your changes, we will discuss how we should move.
Thanks
Regards
JB
On 06/02/2012 11:17 AM, Christian Schneider wrote:
After my rather emotional driven first answer I would like to elaborate
a bit more.
I have put a lot of effort into my implementation of feature reading in
the main. I made sure the code does not introduce any new dependencies
which was one of the issues. I added a test which shows the code works
and also works together with the startup.properties handling. The code
was only about 20 lines so it did not make the Main class really more
complex.
Guillaume correctly listed some unaddressed concerns. So lets look into
the remaining issues inline:
Am 25.05.2012 17:55, schrieb Christian Schneider:
Thanks for the explanations.
I don´t think I can address these concerns while keeping the original
idea of using features like the startup.properties. So I closed the
issue as wont fix.
Christian
Am 25.05.2012 17:34, schrieb Guillaume Nodet:
David said he didn't want xml processing in the bootstrap. This
clearly has not been addressed.
As features are expressed in xml I can not really address this without
completely skipping the feature reading. This was the main reason why I
closed the issue. I hoped my simple implementation convinced David that
xml handling is not so bad after all. As he did not express any concerns
about my patch I thought this is resolved.
Andreas said there's no value in your change because the file is now
fully generated. This hasn't been addressed.
The value is in not having to generate the startup.properties. This
would make the karaf maven plugin simpler as the code could be removed
there. This is of course just my personal opinion. So I am not sure how
to resolve that concern.
I said only supporting a subset of features schema is a problem. This
hasn't been addressed.
I answered in the issue that the subset of a feature that is handled is
quite equivalent in what we can do in the startup.properties file. As
the framwork feature is the core of karaf I think we can
live with not having the ability to reference other features or define
configs. These additional features would make the feature handling too
complex for the Main class. So I think we can argue that it is necessary
and not a big problem to only support a subset.
I may have missed something. But when people disagree, letting time
pass does not usually change things.
AFAIK, those concerns has been raised on the patch you uploaded, so
there's something wrong here.
There was only one comment on the patch from Guillaume. It was the last
one in the list about not handling all possibilities of a feature. As I
thought this was not a severe issue and adddtionall features could be
added later if necessary I committed the patch. No one else commented or
reviewed the patch. So I thought this was a kind of silent approval. So
I really had a good feeling about the commit and was very disappointed
about Guillaumes -1 as I thought I really did everything as well as I
could by supplying a patch and waiting more than 4 days.
@Guillaume so what is your proposal. How should I have handled this
issue? Should I have given the jira issue more time so more people could
review the patch? Should I ask for positive approval which is rather
uncommon at apache?
Christian
--
Jean-Baptiste Onofré
[email protected]
http://blog.nanthrax.net
Talend - http://www.talend.com
