[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #43: [MBUILDCACHE-46] Add maven.build.cache.remote.enabled parameter

["AlexanderAshitkin (via GitHub)" <gi...@apache.org>]

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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org