Devmode: introduce quarkus.live-reload.watched-resources

[famod]

Resolves #10234

This is WIP because there are some things left to clarify and to implement:

• Test is missing. I'd extend DevMojoIT. Is this ok?
• Not sure about the devmode ConfigRoot as there is also LiveReloadConfig (which seems like a workaround judging from its JavaDoc)
• additional-watched-resources could be shortened to extra-watched-resources or ...? In case more "watched" aspects should be configurable in the future (e.g. Enable quarkus:dev to hot-reload resources from non-standard paths #10298?) then maybe watched.additional-resources or similar would be better.
• JavaDoc for the new item is missing
• Not sure about the ConfigPhase.

PS: I remove the duplicate toAbsolutePath() from Paths.get(".env").toAbsolutePath().toAbsolutePath().toString().

[gsmet]

Thanks for that! I added one comment.

Also, I think it would be better to have a separate test using the QuarkusDevModeTest facility. You have an example in DevModeConstraintValidationTest for instance.

[famod]

I think I'm done. I removed the following entirely:

/**
 * This is used currently only to suppress warnings about unknown properties
 * when the user supplies something like: -Dquarkus.live-reload.password=secret
 *
 * TODO refactor code to actually use these values
 */

because the two properties are actually used. Seemed like a leftover.

[famod]

Btw, it would be great to see this in 1.6.0. I know it's late, but IMO this has a very low risk of breaking anything.

[jaikiran]

More of a curiosity than a review comment - For types like List does it add value to wrap it add around a Optional? If no config values are provided for the property, then the config management framework can perhaps just create an empty list?

[gsmet]

Ah ah, you want to talk to @dmlloyd about that :).

If you want to allow the empty list, you need an Optional.

[geoand]

I don't remember the exact reason (although David certainly does and perhaps @ia3andy to whom David had explained it recently), but what Guillaume says is spot on.

[ia3andy]

@dmlloyd will confirm but from what I remember, it was to be consistant for all different types, it's not Optional if not specified as an Optional whatever the type. and you can always decide to convert the empty optional to an empty list when consuming it.

I have a task in my todos to document that a bit better, but it's always sliding, I need to get to it 😅, aw and I created an issue:
#8306

[jaikiran]

Hello @famod, does this enhancement allow adding a directory as an additional watched resource? If yes, then looking at the hot deployment infrastructure I'm not 100% sure if this works in the current form i.e. if I change some file within the directory/sub-directory will it trigger the live reload?
If directories aren't allowed to be configured for this property, perhaps we should check such values and either log a warning or throw an exception?

[famod]

Hi @jaikiran. Right now you would be able to set a name that matches a directory and the directory is added to RuntimeUpdatesProcessor.watchedFileTimestamps, but changing a file in such a directory will not trigger a restart.
I'm going to extend the JavaDoc.

As for logging or throwing an exception, this is a bit complicated as a "name" could theoretically be a directory in one DevModeContext.ModuleInfo but a regular file in another one.
(I am talking about io.quarkus.deployment.dev.RuntimeUpdatesProcessor.setWatchedFilePaths(Map<String, Boolean>))

I'll push a second commit with what I think is sensible and you guys can decide if you want to keep it or not...

[famod]

I decided against touching RuntimeUpdatesProcessor because I don't fully understand all things that are going on there, especially the directory handling, e.g.: #5217 (comment)

So I only updated the JavaDoc of the new config item.

[famod]

@gsmet checks are green. Ready for (final?) review. :-)

[famod]

cc @geoand since you had this idea originally.

[geoand]

I'll try and review tomorrow but I can't guarantee it

[geoand]

LGTM, thanks!

[mkouba]

LGTM.

I'd vote for a shorter name though. quarkus.live-reload.watched-resources seems good enough to me. As a reference - in ArC we have quarkus.arc.selected-alternatives which is used to "select" alternatives but these can be selected by @Priority as well...

[famod]

I'd vote for a shorter name though. quarkus.live-reload.watched-resources seems good enough to me.

This is done quickly but let's see what @gsmet says.

PS: Still hoping this is small enough for 1.6.1. 😉

[famod]

@gsmet WDYT?

[gsmet]

Yeah, we can go with Martin's proposal. Just make sure to also update the commit message.

[famod]

@gsmet

Yeah, we can go with Martin's proposal. Just make sure to also update the commit message.

Done. I left "additional" for the Javadoc and the test class names because it kind of made sense to me.

[famod]

Just rebased. Would be nice if someone could merge this...

[gsmet]

Merged, thanks!