Skip to content

fix(jenkins): fix the issue of Retrofit2EncodeCorrectionInterceptor getting added to Jenkins retrofit1 client #1324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

kirangodishala
Copy link
Contributor

  • This PR addresses the issue - Triggering a Jenkins job with parameters replace spaces with + spinnaker#7059
  • The OkHttpClient autowired in JenkinsConfig was coming from RawOkHttpClientConfiguration which is adding the interceptors meant for retrofit2 client. This PR takes care of the issue by getting the OkHttpClient from the right source which is OkHttp3ClientConfiguration.
  • Added a test to demonstrate the issue of Retrofit2EncodeCorrectionInterceptor getting added to the Jenkins retrofit1 client.

…CorrectionInterceptor getting added to retrofit1 client of JenkinsClient
…etting added to retrofit1 client of JenkinsClient.

More details on this issue:
spinnaker/spinnaker#7059
import retrofit.RestAdapter
import spock.lang.Specification;

@SpringBootTest(classes = [JenkinsConfig, RawOkHttpClientConfiguration, OkHttpClientComponents, TestConfiguration, OkHttp3ClientConfiguration],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is RawOkHttpClientConfiguration still required after the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed but it shows the real scenario of two OkHttpClient beans available but JenkinsConfig is using the right one. Let me know if you want me remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm...this is a tough one. Aren't we getting lucky here? RawOkHttpClientConfiguration uses @ConditionalOnMissingBean. What guarantees do we have that JenkinsConfig doesn't pick that up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed RawOkHttpClientConfiguration and converted the test to java.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. What about @ConditionalOnMissingBean though? Aren't we still getting lucky in the main igor code, because Main has:

@SpringBootApplication(
  scanBasePackages = [
    "",
    "com.netflix.spinnaker.igor"
  ],
  exclude = [GroovyTemplateAutoConfiguration]
)

and specifically com.netflix.spinnaker.config because RawOkHttpClientConfiguration is in

package com.netflix.spinnaker.config.okhttp3;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If RawOkHttpClientConfiguration gets processed first, I guess we end up with two OkHttpClient beans, one named okhttp3Client (from RawOkHttpClientConfiguration) and one named okHttpClient (from JenkinsConfig). And since the rest of the code in JenkinsConfig uses the variable name okHttpClient (and not okhttp3Client) perhaps things "just work." We're gonna go ahead ...

@kirangodishala kirangodishala force-pushed the fix-jenkins-job-issue branch from 7fe0d06 to 3ff78ca Compare May 9, 2025 19:30
@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label May 12, 2025
@mergify mergify bot added the auto merged label May 12, 2025
@mergify mergify bot merged commit 840fdb6 into spinnaker:release-1.37.x May 12, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants