fabric8io-images / s2i

OpenShift S2I images for Java and Karaf applications
Apache License 2.0
70 stars 84 forks source link

[ENTESB-10469]modify image script to honor the mirrors/proxy in confi… #245

Closed ffang closed 4 years ago

ffang commented 4 years ago

…guration/settings.xml of each quickstarts

rhuss commented 4 years ago

Any particular why you move up the code section which evaluates the settings.xml ?

ffang commented 4 years ago

Hi @rhuss ,

I moved that up cause I want to override the /home/jboss/.m2/settings.xml in pod with the configuration/settings.xml in quickstarts. So the later

<mirrors>
   <!-- ### configured mirrors ### -->
 </mirrors>

 <proxies>
   <!-- ### configured http proxy ### -->
 </proxies>

substitution can also apply to configuration/settings.xml in each quickstarts. Currently if there is a configuration/settings.xml in the quickstart, the para like HTTP_PROXY_HOST|HTTP_PROXY_PORT|MAVEN_MIRROR_URL will simply be ignored cause the substitution only happens to pod /home/jboss/.m2/settings.xml but not to configuration/settings.xml in each quickstarts|git projects

Thanks! Freeman

rhuss commented 4 years ago

So then you expect that the user-provided settings.xml also has the placeholders like <!-- ### configured http proxy ### --> contained .

If not (can be easily the case as this is not guaranteed), the parameters will be also ignored. I wonder whether in this case we should check for the placeholders to be present and if not, throw out an error, explaining what to do. Alternatively, we could try to add it ot the user-provided settings.xml even without placeholders.

Tbh, I would think that if a user provides a settings.xml then she would add the proxies already into this provided settings.xml (i.e. either use the env vars or add them directly to the settings).

ffang commented 4 years ago

@rhuss , yes, I've added such placeholders to all fabric8-quickstarts.

The requirement is actually from QE(you can take a look at long conversation in ENTESB-10694) to get more background). QE needs to test fabric8-quickstarts before GA, that's why they need to be able to pass in MAVEN_MIRROR_URL to point to Red Hat internal maven repo(like indy during the release process). And the MAVEN_MIRROR_URL should be(or can't as it can change) put into configuration/settings.xml in each quickstarts beforehand.

ffang commented 4 years ago

@rhuss Did you see my explanation? Is it good for you to merge? Thanks!

ffang commented 4 years ago

Thanks @rhuss for the feedback!

Revised accordingly