Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add required properties for expansion in the build ConfigSource #14809

Merged
merged 1 commit into from
Apr 5, 2021

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Feb 3, 2021

While this adds any required property for expansion, there are some situations we cannot really control:

  • Since expansion properties are set by other plugins, we may still fail if these plugin do not execute before our own.
  • In a typical case where you run just the Quarkus plugin with quarkus:dev we don't have any generated expansion property available, so you still get the error. In this particular case, we can discuss if we need to eagerly initialize the ConfigRoot for the ContainerImage, since it should not be required in quarkus:dev.
    • This can be avoided by just setting the property in the %dev profile.

@ghost ghost added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven labels Feb 3, 2021
@radcortez
Copy link
Member Author

/cc @rmanibus

@radcortez radcortez requested a review from aloubyansky February 3, 2021 22:39
@radcortez radcortez force-pushed the fix-14788 branch 2 times, most recently from d0115d5 to 4d638ec Compare February 3, 2021 22:44
@rmanibus
Copy link
Contributor

rmanibus commented Feb 4, 2021

Thank you very much, I think this will be really useful at least for build time config properties.

The problems with settings that properties in application.properties are :

  • in a multi modules project this make a lot of duplication
  • we can't really have "generated" values, like the git branch case (this can be achieved with maven filtering, but then the filtering must be performed before to have a correct result)
  • I préfere not to see build time config after the build

There is really something with quarkus:dev, many times It would have been useful to execute other tasks / plugins around.

Maybe we can advocate on maven side to implement some kind of "custom phase" ?

Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @radcortez, thanks! A little testcase with a non-quarkus. property in the pom would be good to add though.

@aloubyansky
Copy link
Member

We should add the same in Gradle, btw.

@aloubyansky
Copy link
Member

Maybe we can advocate on maven side to implement some kind of "custom phase" ?

How would that work @rmanibus ?
Perhaps a less elegant alternative would be to add a possibility to provide a list of plugins that should be invoked from the dev mojo.

@rmanibus
Copy link
Contributor

rmanibus commented Feb 4, 2021

@aloubyansky
I was thinking of a user defined phase that is not in the normal lifecycle but that link plugin together in a single command.

We could also imagine to give it a rank, for example when this phase is invoked execute all phase before compile.

@aloubyansky
Copy link
Member

Would you mind sketching an example pom config? I guess in the dev mojo we could try looking for plugins configured for let's say pre-dev phase and invoking them.

@rmanibus
Copy link
Contributor

rmanibus commented Feb 4, 2021

A typical use case would be to use to invoke docker plugin to start a dB and then inject some data with dbunit plugin. I will draft something about that !

@aloubyansky
Copy link
Member

@rmanibus
Copy link
Contributor

rmanibus commented Feb 4, 2021

@aloubyansky in my reproducer regarding this issue, running quarkus:dev will fail because it can not expand quarkus.container-image.tag even if it's not formally required to start the Dev mode. I think we managed to pass this by setting a default value that get overridden by the git plugin, so the branch is wrong during quarkus dev but it has no impact because it is not used.

I think it would be cleaner to execute everything before compile when running quarkus:dev

Thanks for the link, I will take a look !

@radcortez
Copy link
Member Author

There is really something with quarkus:dev, many times It would have been useful to execute other tasks / plugins around.

Maybe we can advocate on maven side to implement some kind of "custom phase" ?

This is very tricky, because the amount of combinations and plugins is huge and you have no idea which ones the user wants to execute or not from our perspective. It would have to be the user to state that somehow.

I think we managed to pass this by setting a default value that get overridden by the git plugin, so the branch is wrong during quarkus dev but it has no impact because it is not used.

Yes, that is one way to do it. I usually set all the properties I'm going to need in the properties section of the POM anyway, even with empty values, just because these plugins generated properties are not recognised by the IDE and they don't have auto completion :)

@gsmet
Copy link
Member

gsmet commented Feb 5, 2021

@radcortez do you have plans to add a test or should we merge as is?

@radcortez
Copy link
Member Author

@radcortez do you have plans to add a test or should we merge as is?

Yes, I'll add some in the evening. Is that fine for the Monday release, or do you need it earlier?

@aloubyansky
Copy link
Member

Perfect @radcortez, thanks!

@radcortez
Copy link
Member Author

Perfect @radcortez, thanks!

Great. I didn't do that before, because without the fix the build would just fail and I thought it was sufficient :)

Base automatically changed from master to main March 12, 2021 15:55
@gsmet
Copy link
Member

gsmet commented Mar 17, 2021

Rebased.

@gsmet gsmet merged commit 1b81f10 into quarkusio:main Apr 5, 2021
@quarkus-bot quarkus-bot bot added this to the 1.14 - main milestone Apr 5, 2021
@radcortez radcortez deleted the fix-14788 branch May 26, 2021 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven
Projects
None yet
4 participants