Properly espace properties in archetype generation

[gsmet]

Supposed to supersede #10829 .

@famod I'm not sure I understand completely what you did in your PR but this looks like a far less intrusive patch and IMHO is correct. Could you have a look?

Thanks!

[geoand]

I assume this is correct, but I am really lacking context here :)

[famod]

For an absolute minimal quickfix that is totally focussing only on maven.home, this would suffice.

But given how the azure-functions-http archetype is working and given my other PR #10836 (that builds on top of #10829) this would not suffice, IMHO.

Btw, the generation produces:

    <properties>
        <!-- ... -->
        <compiler-plugin.version>3.8.1</compiler-plugin.version>
        <!-- ... -->
    </properties>
<!-- ... -->
            <plugin>
                <artifactId>maven-compiler-plugin</artifactId>
                <version>3.8.1</version>
            </plugin>

so we are leaving the users under the impression that they can change the property for the compiler plugin to get another version but this won't work.

[gsmet]

@famod you're right, my patch was incomplete. I fixed it.

Note that it doesn't fix the bom issue. I just want the escaping to work OK.

As mentioned I'm not sure I understand everything you did in #10829 but I think just escaping the properties is the way to go.

Agreed the Azure one is working differently but that will be for another day.

[famod]

@gsmet The current escaping mechanism is just too much voodoo in my opinion (YAGNI).
It is just not necessary and the archetype of azure-functions-http proves that.
You have to remember to add \ which you do not have to with my approach.

I mean it is your decision but can I help you to understand what I did in #10829?
I actually made it much easier...

PS: The story changes if we really want to filter things at build time (when the archetype is built). But that is not the case today.

[gsmet]

Yeah, I think I prefer the escaping thing. It's not that voodoo to me, you escape the properties you do not want filtering to replace.

I'm working on porting your other patch to this approach, will push it to this PR soon.

[famod]

Ok.

I'd just like to point out that in terms of Maven less is more. If you can stick to the convention: do it!
It is not the first time I saw inconsistent Maven setup code in this big repo and the more verbose the setup gets the more likelier inconsistencies become.

[gsmet]

I agree. I like consistency but I'm really worried the approach chosen for the Azure archetype will bite us at some point.

I included your second commit in the PR adjusted to the new approach with some changes:

• stick to the quarkus-bom for now as we are in the core repo. Having a dependency to universe is weird and typically wouldn't work if we were testing the artifact properly. We plan to include the Lambda generation in code.quarkus.io at some point so that would resolve that point.
• I went back to 1.8. Changing this to 11 should be done in another PR as I won't backport it to 1.6.1. Also we need to keep Java 8 for the Azure archetype as Java 11 is not supported yet. Do you mind opening another PR targeting 1.7 with this only change? Thanks!

Could you have a look? I tested the creation of each archetype and it was OK but a second pair of eyes could help.

[gsmet]

That being said, with #10849 coming, we will not need the second commit in 1.6.1. It's probably better to keep it for 1.7 altogether.

[famod]

I included your second commit in the PR

Just to understand correctly: So this PR is replacing both #10829 and #10836?

Also we need to keep Java 8 for the Azure archetype as Java 11 is not supported yet. Do you mind opening another PR targeting 1.7 with this only change?

You mean only 1.8 -> 11 in pom.xml? But what is actually needed to support 11 for Azure?
Sorry for these silly questions but I am a bit slow today and I am not an Azure expert. 😉

Could you have a look? I tested the creation of each archetype and it was OK but a second pair of eyes could help.

Unfortunately I don't have time righty now to re-test the archetypes but I'll look at the code for sure.

That being said, with #10849 coming, we will not need the second commit in 1.6.1. It's probably better to keep it for 1.7 altogether.

Makes perfect sense!

[famod]

Unfortunately I don't have time righty now to re-test the archetypes but I'll look at the code for sure.

LGTM!

[gsmet]

Yes, it replaces both.

You mean only 1.8 -> 11 in pom.xml?

Yes.

But what is actually needed to support 11 for Azure?

Apparently Azure Functions do not support 11 yet. So we need to stick to 8 for them and we will upgrade later.

[geoand]

Adding an approval so you can backport whenever you need to.

Y'all added more information than I want to know and absolutely trust you to find the best solution

[gsmet]

I partially backported it: only the first commit has been backported to 1.6.1.Final.