LGTM now, thanks for the work! Romain Manni-Bucau @rmannibucau <https://twitter.com/rmannibucau> | Blog <https://rmannibucau.metawerx.net/> | Old Blog <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book <https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 29 sept. 2020 à 08:10, Jean-Baptiste Onofre <j...@nanthrax.net> a écrit : > Done, I’m doing a full test. > > Regards > JB > > > Le 29 sept. 2020 à 07:59, Romain Manni-Bucau <rmannibu...@gmail.com> a > écrit : > > > > Hi JB, > > > > for the env part it looks exactly what I would expect but for system > > properties I wouldnt normalize the key (uppercase+replace dots by > > underscrores), it should just be "pid + "." + key" I think. Normalization > > is not only to look like common practise but also cause dots are not > > accepted in env var IIRC. Since it is not the case for system props, no > > reason to apply that transformation IMHO. > > Last small note is that env should win over system properties so second > > "if" should be preceeded by an "else" IMHO. > > > > Hope it makes sense. > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <https://rmannibucau.metawerx.net/> | Old Blog > > <http://rmannibucau.wordpress.com> | Github < > https://github.com/rmannibucau> | > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > > < > https://www.packtpub.com/application-development/java-ee-8-high-performance > > > > > > > > Le mar. 29 sept. 2020 à 07:16, Jean-Baptiste Onofre <j...@nanthrax.net> a > > écrit : > > > >> By the way, ConfigurationPlugin is R7 so Karaf 4.3.x. > >> > >> For Karaf 4.2.x, I will use the ${env} notation in cfg files using the > >> same var names as working on 4.3. > >> > >> Regards > >> JB > >> > >>> Le 29 sept. 2020 à 05:56, Jean-Baptiste Onofre <j...@nanthrax.net> a > >> écrit : > >>> > >>> Hi, > >>> > >>> I’ve updated the PR to use ConfigurationPlugin. > >>> > >>> NB: when using ConfigurationPlugin, if you use > >> configuration.getProperties() you will have the default values (not > >> interpolated). To get the actual property values, you have to use > >> configuration.getProcessedProperties(). With > >> configuration.getProcessedProperties(), you will the interpolated value. > >>> In order to avoid to confuse users, I also updated configRepository and > >> config:* commands to use getProcessedProperties() by default. > >>> > >>> I’m doing a pass on other Karaf modules. > >>> > >>> Regards > >>> JB > >>> > >>>> Le 28 sept. 2020 à 11:39, Romain Manni-Bucau <rmannibu...@gmail.com> > a > >> écrit : > >>>> > >>>> +1, I'd try to push for a built-in support - > >>>> (<pid>.<property_name>).toUpperCase(ROOT).replace('.', '_') - to make > it > >>>> generic and not specific to some entries. It would also enable to not > >> have > >>>> to rename these env variables or have to maintain some fallbacks. > >>>> What about just implementing a felix config admin configuration > plugin? > >>>> sounds less invasive and better on the long run? Did you give it a > try? > >>>> > >>>> Romain Manni-Bucau > >>>> @rmannibucau <https://twitter.com/rmannibucau < > >> https://twitter.com/rmannibucau>> | Blog > >>>> <https://rmannibucau.metawerx.net/ <https://rmannibucau.metawerx.net/ > >> > >> | Old Blog > >>>> <http://rmannibucau.wordpress.com <http://rmannibucau.wordpress.com/ > >> > >> | Github <https://github.com/rmannibucau < > https://github.com/rmannibucau>> > >> | > >>>> LinkedIn <https://www.linkedin.com/in/rmannibucau < > >> https://www.linkedin.com/in/rmannibucau>> | Book > >>>> < > >> > https://www.packtpub.com/application-development/java-ee-8-high-performance > >> < > >> > https://www.packtpub.com/application-development/java-ee-8-high-performance > >>>> > >>>> > >>>> > >>>> Le lun. 28 sept. 2020 à 11:12, Jean-Baptiste Onofre <j...@nanthrax.net > >> <mailto:j...@nanthrax.net>> a > >>>> écrit : > >>>> > >>>>> Fair enough (and I agree ;)). > >>>>> > >>>>> Regards > >>>>> JB > >>>>> > >>>>>> Le 28 sept. 2020 à 11:09, Grzegorz Grzybek <gr.grzy...@gmail.com> a > >>>>> écrit : > >>>>>> > >>>>>> I'd stick to explicit approach for now and leave "implicit" one for > >>>>> later ;) > >>>>>> > >>>>>> regards > >>>>>> Grzegorz Grzybek > >>>>>> > >>>>>> pon., 28 wrz 2020 o 11:08 Jean-Baptiste Onofre <j...@nanthrax.net > >> <mailto:j...@nanthrax.net> <mailto: > >>>>> j...@nanthrax.net <mailto:j...@nanthrax.net>>> napisał(a): > >>>>>> > >>>>>>> Just to be clear: today I’m using an "explicit" approach, where the > >>>>> config > >>>>>>> contains something like: > >>>>>>> > >>>>>>> sshPort=${env:KARAF_SSH_PORT:-8101} > >>>>>>> > >>>>>>> But we can also have "implicit" approach (with some more changes > >>>>> required). > >>>>>>> > >>>>>>> Regards > >>>>>>> JB > >>>>>>> > >>>>>>>> Le 28 sept. 2020 à 11:07, Jean-Baptiste Onofre <j...@nanthrax.net > >> <mailto:j...@nanthrax.net>> a > >>>>>>> écrit : > >>>>>>>> > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> At beginning, I did a very simple change in Karaf Main: for > >> instance, > >>>>>>> you would be able to do > >>>>>>>> > >>>>>>>> Bin/karaf -Dpid:prop=value > >>>>>>>> > >>>>>>>> And I init the configuration file with the value. > >>>>>>>> > >>>>>>>> However, system properties are not super easy with docker. > >>>>>>>> > >>>>>>>> That’s why I preferred the env variable approach. > >>>>>>>> > >>>>>>>> Now, about env variable, I just leverage what we already have in > >> Karaf > >>>>>>> (just updating the default configuration file). > >>>>>>>> > >>>>>>>> I can do a new iteration where (in configadmin repository), I’m > >>>>> checking > >>>>>>> ALL env variables to find one matching. > >>>>>>>> It would mean something like: > >>>>>>>> > >>>>>>>> $ export KARAF.MY_PID.prop=value > >>>>>>>> > >>>>>>>> For instance. > >>>>>>>> > >>>>>>>> We would need a "env variable format". > >>>>>>>> > >>>>>>>> Thoughts ? > >>>>>>>> > >>>>>>>> Regards > >>>>>>>> JB > >>>>>>>> > >>>>>>>>> Le 28 sept. 2020 à 10:59, Grzegorz Grzybek <gr.grzy...@gmail.com > >> <mailto:gr.grzy...@gmail.com> > >>>>>>> <mailto:gr.grzy...@gmail.com <mailto:gr.grzy...@gmail.com> > <mailto: > >> gr.grzy...@gmail.com <mailto:gr.grzy...@gmail.com>>>> a écrit : > >>>>>>>>> > >>>>>>>>> Hello > >>>>>>>>> > >>>>>>>>> Good idea! > >>>>>>>>> > >>>>>>>>> Shouldn't configadmin do it by default? > >>>>>>>>> > >>>>>>>>> Like dissect "KARAF_SSH_PORT" or similar env variables into: > >>>>>>>>> - prefix (KARAF_) - rejected > >>>>>>>>> - PID pointer (e.g., SSH → org.apache.karaf.shell) > >>>>>>>>> - property (e.g PORT → sshPort) > >>>>>>>>> ? > >>>>>>>>> > >>>>>>>>> This way it could be done in one place... Just my random > >> observation, > >>>>>>>>> because I can't dig this problem further for now ;) > >>>>>>>>> > >>>>>>>>> regards > >>>>>>>>> Grzegorz Grzybek > >>>>>>>>> > >>>>>>>>> pon., 28 wrz 2020 o 10:51 Jean-Baptiste Onofre <j...@nanthrax.net > >> <mailto:j...@nanthrax.net> > >>>>> <mailto:j...@nanthrax.net <mailto:j...@nanthrax.net>> > >>>>>>> <mailto:j...@nanthrax.net <mailto:j...@nanthrax.net> <mailto: > >> j...@nanthrax.net <mailto:j...@nanthrax.net>>> <mailto: > >>>>> j...@nanthrax.net <mailto:j...@nanthrax.net> <mailto:j...@nanthrax.net > >> <mailto:j...@nanthrax.net>> <mailto:j...@nanthrax.net <mailto: > j...@nanthrax.net> > >> <mailto: > >>>>> j...@nanthrax.net <mailto:j...@nanthrax.net>>>>> > >>>>>>> napisał(a): > >>>>>>>>> > >>>>>>>>>> Hi guys, > >>>>>>>>>> > >>>>>>>>>> In order to easily use Karaf in docker container, I created the > >>>>>>> following > >>>>>>>>>> PR: > >>>>>>>>>> > >>>>>>>>>> https://github.com/apache/karaf/pull/1203 < > >> https://github.com/apache/karaf/pull/1203> < > >>>>> https://github.com/apache/karaf/pull/1203 < > >> https://github.com/apache/karaf/pull/1203>> < > >>>>>>> https://github.com/apache/karaf/pull/1203 < > >> https://github.com/apache/karaf/pull/1203> < > >>>>> https://github.com/apache/karaf/pull/1203 < > >> https://github.com/apache/karaf/pull/1203>>> < > >>>>>>> https://github.com/apache/karaf/pull/1203 < > >>>>> https://github.com/apache/karaf/pull/1203> < > >>>>>>> https://github.com/apache/karaf/pull/1203 < > >>>>> https://github.com/apache/karaf/pull/1203>>> < > >>>>>>>>>> https://github.com/apache/karaf/pull/1203 < > >>>>>>> https://github.com/apache/karaf/pull/1203> < > >>>>>>> https://github.com/apache/karaf/pull/1203 < > >>>>>>> https://github.com/apache/karaf/pull/1203>>> > >>>>>>>>>> > >>>>>>>>>> The purpose is allow use to override some configuration > properties > >>>>>>> using > >>>>>>>>>> env variables. > >>>>>>>>>> > >>>>>>>>>> For instance, if you want to use 8102 instead of default 8101 > ssh > >>>>> port > >>>>>>>>>> number, you can do: > >>>>>>>>>> > >>>>>>>>>> $ export KARAF_SSH_PORT=8102 > >>>>>>>>>> $ bin/karaf > >>>>>>>>>> > >>>>>>>>>> I listed the environment variables here: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>> > >>>>> > >> > https://github.com/apache/karaf/blob/4eda325e73d1d7dfbc0508258bde582e93d4f05e/manual/src/main/asciidoc/user-guide/configuration.adoc > >> < > >> > https://github.com/apache/karaf/blob/4eda325e73d1d7dfbc0508258bde582e93d4f05e/manual/src/main/asciidoc/user-guide/configuration.adoc > >>> > >>>>> < > >>>>> > >> > https://github.com/apache/karaf/blob/4eda325e73d1d7dfbc0508258bde582e93d4f05e/manual/src/main/asciidoc/user-guide/configuration.adoc > >>>>>> > >>>>>>> < > >>>>>>> > >>>>> > >> > https://github.com/apache/karaf/blob/4eda325e73d1d7dfbc0508258bde582e93d4f05e/manual/src/main/asciidoc/user-guide/configuration.adoc > >>>>>>>> > >>>>>>>>>> < > >>>>>>>>>> > >>>>>>> > >>>>> > >> > https://github.com/apache/karaf/blob/4eda325e73d1d7dfbc0508258bde582e93d4f05e/manual/src/main/asciidoc/user-guide/configuration.adoc > >>>>>>> < > >>>>>>> > >>>>> > >> > https://github.com/apache/karaf/blob/4eda325e73d1d7dfbc0508258bde582e93d4f05e/manual/src/main/asciidoc/user-guide/configuration.adoc > >>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> I didn’t define environment variables for all properties. If you > >> see > >>>>>>> some > >>>>>>>>>> properties that it would be interesting to override by env > >> variables, > >>>>>>>>>> please let me know, I will update the PR. > >>>>>>>>>> > >>>>>>>>>> NB: I will also update pax* to use similar approach. > >>>>>>>>>> > >>>>>>>>>> Thoughts ? > >>>>>>>>>> > >>>>>>>>>> Regards > >>>>>>>>>> JB > >>> > >> > >> > >
