AlexanderAshitkin commented on code in PR #43:
URL:
https://github.com/apache/maven-build-cache-extension/pull/43#discussion_r1125516876
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -173,11 +173,14 @@ public CacheResult findCachedBuild(
LOGGER.info("Attempting to restore project {} from build cache",
projectName);
// remote build first
- result = findCachedBuild(mojoExecutions, context);
+ if (cacheConfig.isRemoteCacheEnabled()) {
Review Comment:
Please write integration tests for this flag. In pr #34, I added
Wiremock-based integration tests to verify the remote cache behavior. The same
approach could be reused here. It will be good to see a test that checks that
remote is disabled.
##########
src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java:
##########
@@ -452,28 +453,29 @@ public boolean isEnabled() {
@Override
public boolean isRemoteCacheEnabled() {
checkInitializedState();
- return getRemote().getUrl() != null && getRemote().isEnabled();
Review Comment:
Though nothing changed here, the `RemoteCacheEnabled` flag intuitively
should disable all the remote interactions. So all the flags below should be
preconditioned on the `remote. enabled`. Again, there is no regression in the
current form, and this is an optional consideration.
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
