Copilot commented on code in PR #395:
URL:
https://github.com/apache/maven-build-cache-extension/pull/395#discussion_r3608847608
##########
src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java:
##########
@@ -183,9 +197,16 @@ public void execute(
.isEmpty()) {
LOGGER.debug("Cache storing is skipped since there was
no \"clean\" phase.");
} else {
- final Map<String, MojoExecutionEvent> executionEvents =
- mojoListener.getProjectExecutions(project);
- cacheController.save(result, mojoExecutions,
executionEvents);
+ // Only save cache if there are validation-time events
to store
+ // When running only clean phase, there are no
cacheable mojos
+ if (result.getValidationTimeEvents() == null
+ || result.getValidationTimeEvents().isEmpty())
{
+ LOGGER.debug("Skipping cache storage for {} - no
cacheable mojos executed", projectName);
+ } else {
+ LOGGER.debug(
+ "Using validation-time properties for
cache storage (consistent lifecycle point)");
+ cacheController.save(result, mojoExecutions,
result.getValidationTimeEvents());
+ }
Review Comment:
The save path currently just logs and skips cache storage when
validation-time events are null/empty. That contradicts the PR description
(“MUST exist … fail with AssertionError”) and can also hide partial capture
failures: if a mojo with tracked properties is missing from the map, those
expected values will be absent in the cache and later comparisons fall back to
defaults, potentially producing false cache hits. Consider enforcing that
validation-time events are present for every cacheable mojo that has tracked
properties, and fail fast (or at least refuse to save) when they’re missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
