refactor(retrofit2): addressed review comments

echo-core/src/test/groovy/com/netflix/spinnaker/echo/services/Front50ServiceSpec.groovy

@@ -4,16 +4,19 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.github.tomakehurst.wiremock.WireMockServer
 import com.github.tomakehurst.wiremock.core.WireMockConfiguration
 import com.github.tomakehurst.wiremock.client.WireMock
+import com.google.common.collect.ImmutableList
 import com.netflix.spinnaker.config.DefaultServiceEndpoint
 import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider
 import com.netflix.spinnaker.config.okhttp3.InsecureOkHttpClientBuilderProvider
+import com.netflix.spinnaker.echo.model.Trigger
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory
 import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall
 import okhttp3.OkHttpClient
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.boot.test.context.SpringBootTest
 import retrofit2.Retrofit
-import retrofit2.converter.jackson.JacksonConverterFactory;
+import retrofit2.converter.jackson.JacksonConverterFactory
+import spock.lang.Ignore;
 import spock.lang.Specification
 import spock.util.concurrent.BlockingVariable
 
@@ -109,6 +112,22 @@ class Front50ServiceSpec extends Specification {
     pipelines.first().parallel
   }
 
+  @Ignore
+  def "list properties are immutable"() {
+    given:
+    def pipelines = front50Service.getPipelines()
+    def pipeline = pipelines.find { it.application == "kato" }
+
+    expect:
+    pipeline.triggers instanceof ImmutableList
+
+    when:
+    pipeline.triggers << Trigger.builder().enabled(false).type('jenkins').master('foo').job('bar').propertyFile('baz').build()
+
+    then:
+    thrown UnsupportedOperationException
+  }
+
   private Front50Service front50Service(String baseUrl){
     new Retrofit.Builder()
       .baseUrl(baseUrl)

echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/notification/InteractiveNotificationCallbackHandler.groovy

@@ -97,6 +97,7 @@ class InteractiveNotificationCallbackHandler {
 
     // TODO(lfp): error handling (retries?). I'd like to respond to the message in a thread, but
     //  have been unable to make that work. Troubleshooting with Slack support.
+    // TODO(lfp): need to retrieve user's accounts to pass in X-SPINNAKER-ACCOUNTS
     final ResponseBody response = Retrofit2SyncCall.execute(spinnakerService.notificationCallback(callback, callback.getUser()));
     log.debug("Received callback response from downstream Spinnaker service: " + response.string());
 

echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/pagerduty/PagerDutyNotificationService.groovy

@@ -19,10 +19,12 @@ package com.netflix.spinnaker.echo.pagerduty
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.echo.api.Notification
+import com.netflix.spinnaker.echo.config.PagerDutyConfigurationProperties
 import com.netflix.spinnaker.echo.controller.EchoResponse
 import com.netflix.spinnaker.echo.notification.NotificationService
 import com.netflix.spinnaker.echo.services.Front50Service
 import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import groovy.transform.InheritConstructors
 import groovy.util.logging.Slf4j
@@ -46,8 +48,8 @@ class PagerDutyNotificationService implements NotificationService {
   @Autowired
   PagerDutyService pagerDuty
 
-  @Value('${pager-duty.token:}')
-  String token
+  @Autowired
+  PagerDutyConfigurationProperties pagerDutyConfigurationProperties
 
   @Autowired
   Front50Service front50Service
@@ -65,7 +67,7 @@ class PagerDutyNotificationService implements NotificationService {
     notification.to.each { serviceKey ->
       try {
         Map response = Retrofit2SyncCall.execute(pagerDuty.createEvent(
-          "Token token=${token}",
+          "Token token=${pagerDutyConfigurationProperties.token}",
           new PagerDutyService.PagerDutyCreateEvent(
             service_key: serviceKey,
             client: "Spinnaker (${notification.source.user})",
@@ -82,10 +84,20 @@ class PagerDutyNotificationService implements NotificationService {
           pdErrors.put(serviceKey, response.message)
         }
       } catch (SpinnakerServerException e){
+        def errorMessage = null
+        if (e instanceof SpinnakerHttpException){
+          Map<String, Object> errorResponse = ((SpinnakerHttpException) e).responseBody
+          if (errorResponse != null) {
+            if (errorResponse.errors && errorResponse.errors.size() > 0) {
+              errorMessage = errorResponse.errors.join(", ")
+            }
+          }
+        }
+        errorMessage = errorMessage == null ? e.message : errorMessage
         log.error('Failed to send page {} {} {}',
           kv('serviceKey', serviceKey), kv('message',
           notification.additionalContext.message),
-          kv('error', e.message)
+          kv('error', errorMessage)
         )
         errors.put(serviceKey, e.message)
       }

echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy

@@ -22,7 +22,6 @@ import com.netflix.spinnaker.echo.controller.EchoResponse
 import com.netflix.spinnaker.echo.notification.NotificationService
 import com.netflix.spinnaker.echo.notification.NotificationTemplateEngine
 import groovy.util.logging.Slf4j
-import okhttp3.ResponseBody
 import org.springframework.beans.factory.annotation.Qualifier
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
 import org.springframework.stereotype.Component
@@ -63,7 +62,6 @@ class SlackNotificationService implements NotificationService {
           new SlackAttachment(subject, text, (InteractiveActions)notification.interactiveActions),
           address, true)
       }
-      log.trace("Received response from Slack: {}", response?.string())
     }
 
     new EchoResponse.Void()

echo-notifications/src/main/java/com/netflix/spinnaker/echo/config/PagerDutyConfig.java

@@ -21,31 +21,27 @@
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
+import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import retrofit2.Retrofit;
 import retrofit2.converter.jackson.JacksonConverterFactory;
 
 @Configuration
+@EnableConfigurationProperties(PagerDutyConfigurationProperties.class)
 @ConditionalOnProperty("pager-duty.enabled")
 public class PagerDutyConfig {
   private static final Logger log = LoggerFactory.getLogger(PagerDutyConfig.class);
 
-  private final String endpoint;
-
-  public PagerDutyConfig(
-      @Value("${pager-duty.endpoint:https://events.pagerduty.com}") String endpoint) {
-    this.endpoint = endpoint;
-  }
-
   @Bean
-  PagerDutyService pagerDutyService(OkHttp3ClientConfiguration okHttpClientConfig) {
+  PagerDutyService pagerDutyService(
+      OkHttp3ClientConfiguration okHttpClientConfig,
+      PagerDutyConfigurationProperties pageDutyProps) {
     log.info("Pager Duty service loaded");
 
     return new Retrofit.Builder()
-        .baseUrl(endpoint)
+        .baseUrl(pageDutyProps.getEndpoint())
         .client(okHttpClientConfig.createForRetrofit2().build())
         .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
         .addConverterFactory(JacksonConverterFactory.create())

echo-notifications/src/main/java/com/netflix/spinnaker/echo/config/PagerDutyConfigurationProperties.java

@@ -0,0 +1,29 @@
+/*
+ * Copyright 2025 OpsMx, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.echo.config;
+
+import lombok.Getter;
+import lombok.Setter;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+@Setter
+@Getter
+@ConfigurationProperties(prefix = "pager-duty")
+public class PagerDutyConfigurationProperties {
+  private String endpoint = "https://events.pagerduty.com";
+  private String token;
+}

echo-notifications/src/main/java/com/netflix/spinnaker/echo/jira/JiraNotificationService.java

@@ -189,11 +189,7 @@ private Map<String, Object> issueRequestBody(Notification notification) {
 
   private Map errors(Exception exception) {
     if (exception instanceof SpinnakerHttpException) {
-      try {
-        return ((SpinnakerHttpException) exception).getResponseBody();
-      } catch (Exception e) {
-        LOGGER.warn("failed retrieving error messages {}", e.getMessage());
-      }
+      return ((SpinnakerHttpException) exception).getResponseBody();
     }
 
     return ImmutableMap.of("errors", exception.getMessage());

echo-notifications/src/main/java/com/netflix/spinnaker/echo/microsoftteams/MicrosoftTeamsService.java

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.echo.microsoftteams;
 
 import com.netflix.spinnaker.config.OkHttp3ClientConfiguration;
+import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
 import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
 import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException;
 import java.net.MalformedURLException;
@@ -42,6 +43,7 @@ public ResponseBody sendMessage(String webhookUrl, MicrosoftTeamsMessage message
         new Retrofit.Builder()
             .baseUrl(webhookUrl)
             .client(okHttp3ClientConfiguration.createForRetrofit2().build())
+            .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
             .addConverterFactory(JacksonConverterFactory.create())
             .build()
             .create(MicrosoftTeamsClient.class);

echo-notifications/src/main/java/com/netflix/spinnaker/echo/notification/CDEventsNotificationAgent.java

@@ -82,10 +82,12 @@ public void sendNotifications(
             response.body() != null ? response.body().string() : "");
       } catch (IOException e) {
         log.info(
-            "Received response from events broker : {} {} for execution id {}",
+            "Received response from events broker : {} {} for execution id {} "
+                + "but unable to serialize the response body: {}",
             response.code(),
             response.message(),
-            executionId);
+            executionId,
+            e.getMessage());
       }
     }
   }

echo-notifications/src/main/java/com/netflix/spinnaker/echo/notification/GithubNotificationAgent.java

@@ -24,6 +24,7 @@
 import com.netflix.spinnaker.echo.github.GithubStatus;
 import com.netflix.spinnaker.kork.core.RetrySupport;
 import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import java.util.Map;
 import java.util.Optional;
 import java.util.regex.Matcher;
@@ -127,16 +128,20 @@ public void sendNotifications(
   }
 
   private String getBranchCommit(String repo, String sha) {
-    GithubCommit message =
-        Retrofit2SyncCall.execute(githubService.getCommit("token " + token, repo, sha));
-
+    GithubCommit message;
+    try {
+      message = Retrofit2SyncCall.execute(githubService.getCommit("token " + token, repo, sha));
+    } catch (SpinnakerServerException e) {
+      return sha;
+    }
     Pattern pattern =
         Pattern.compile(
             "Merge (?<branchCommit>[0-9a-f]{5,40}) into (?<masterCommit>[0-9a-f]{5,40})");
     Matcher matcher = pattern.matcher(message.getCommit().getMessage());
     if (matcher.matches()) {
       return matcher.group("branchCommit");
     }
+
     return sha;
   }
 

echo-notifications/src/main/java/com/netflix/spinnaker/echo/notification/MicrosoftTeamsNotificationAgent.java

@@ -150,7 +150,7 @@ public void sendNotifications(
           response.string());
     } catch (IOException e) {
       log.info(
-          "Received response from Microsoft Teams Webhook  for execution id {} but failed to deserialize",
+          "Received response from Microsoft Teams Webhook for execution id {} but failed to deserialize",
           executionId);
     }
   }

echo-rest/echo-rest.gradle

@@ -19,7 +19,6 @@ dependencies {
   implementation project(':echo-model')
   implementation project(':echo-api')
   implementation project(':echo-core')
-  implementation project(':echo-test')
 
   implementation "commons-codec:commons-codec"
 
@@ -29,6 +28,7 @@ dependencies {
   implementation "javax.validation:validation-api"
   implementation "org.apache.commons:commons-lang3"
 
+  testImplementation project(':echo-test')
   testImplementation "com.github.tomakehurst:wiremock-jre8"
   testImplementation "org.springframework.boot:spring-boot-starter-test"
 }

echo-telemetry/src/main/java/com/netflix/spinnaker/echo/config/TelemetryConfig.kt

@@ -15,11 +15,15 @@
  */
 package com.netflix.spinnaker.echo.config
 
+import com.netflix.spinnaker.config.DefaultServiceEndpoint
 import com.netflix.spinnaker.config.OkHttp3ClientConfiguration
+import com.netflix.spinnaker.config.okhttp3.DefaultOkHttpClientBuilderProvider
 import com.netflix.spinnaker.echo.config.TelemetryConfig.TelemetryConfigProps
 import com.netflix.spinnaker.echo.telemetry.TelemetryService
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory
+import com.netflix.spinnaker.okhttp.OkHttpClientConfigurationProperties
 import de.huxhorn.sulky.ulid.ULID
+import okhttp3.OkHttpClient
 import org.slf4j.LoggerFactory
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
 import org.springframework.boot.context.properties.ConfigurationProperties
@@ -41,12 +45,15 @@ open class TelemetryConfig {
   @Bean
   open fun telemetryService(
     configProps: TelemetryConfigProps,
-    okHttpClientConfig: OkHttp3ClientConfiguration
+    okHttpClientConfig: OkHttp3ClientConfiguration,
+    okHttpClient: OkHttpClient
   ): TelemetryService {
+    val clientProps = OkHttpClientConfigurationProperties(configProps.connectionTimeoutMillis.toLong(), configProps.readTimeoutMillis.toLong())
+    val clientProvider = DefaultOkHttpClientBuilderProvider(okHttpClient, clientProps)
     log.info("Telemetry service loaded")
     return Retrofit.Builder()
       .baseUrl(configProps.endpoint)
-      .client(okHttpClientConfig.createForRetrofit2().build())
+      .client(clientProvider.get(DefaultServiceEndpoint("telemetry", configProps.endpoint)).build())
       .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
       .addConverterFactory(JacksonConverterFactory.create())
       .build()
@@ -65,6 +72,8 @@ open class TelemetryConfig {
     var instanceId = ULID().nextULID()
     var spinnakerVersion = "unknown"
     var deploymentMethod = DeploymentMethod()
+    var connectionTimeoutMillis = 3000
+    var readTimeoutMillis = 5000
 
     class DeploymentMethod {
       var type: String? = null

echo-telemetry/src/test/kotlin/com/netflix/spinnaker/echo/telemetry/TelemetryEventListenerTest.kt

@@ -22,9 +22,13 @@ import com.netflix.spinnaker.echo.api.events.Event
 import com.netflix.spinnaker.echo.api.events.Metadata
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
 import io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry
-import io.mockk.*
+import io.mockk.Called
+import io.mockk.CapturingSlot
+import io.mockk.every
 import io.mockk.impl.annotations.MockK
 import io.mockk.junit5.MockKExtension
+import io.mockk.slot
+import io.mockk.verify
 import okhttp3.MediaType.Companion.toMediaTypeOrNull
 import okhttp3.Request
 import okhttp3.RequestBody
@@ -144,7 +148,7 @@ class TelemetryEventListenerTest {
     val event = createLoggableEvent()
     val request: Request = Request.Builder().url("http://url").build()
     every { telemetryService.log(any()) } throws
-      SpinnakerNetworkException(IOException("timeout"), request)
+      SpinnakerNetworkException(IOException("network error"), request)
     expectCatching {
       telemetryEventListener.processEvent(event)
     }.isSuccess()