Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(web): fix the mismatches in the return types of APIs in gate and orca #1883

Merged
merged 5 commits into from
Mar 1, 2025
Merged

fix(web): fix the mismatches in the return types of APIs in gate and orca #1883

merged 5 commits into from
Mar 1, 2025

Conversation

kirangodishala
Copy link
Contributor

  • While some of the Orca's API have void return types, Gate has Map return type for the corresponding APIs causing the following error :
Failed to process response body: No content to map due to end-of-input\n at [Source: (okhttps.ResponseBody$BomAwareReader); line:1, column:0)
  • Retrofit1 ignored the mismatches but retrofit2 has stricter response handling.
  • This PR adds a test to demonstrate the issue and addressed the mismatches.

@kirangodishala
test(web): add a test to demonstrate the mismatch in api response typ…
a9d3d1b 
…es of gate and orca when delete pipeline execution api is invoked
@kirangodishala kirangodishala requested a review from dbyron-sf March 1, 2025 14:29
dbyron-sf
dbyron-sf reviewed Mar 1, 2025
View reviewed changes
gate-web/src/test/java/com/netflix/spinnaker/gate/service/PipelineServiceTest.java

@Autowired ObjectMapper objectMapper;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this in these tests?

Copy link
Contributor Author

@kirangodishala kirangodishala Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean autowiring of ObjectMapper? It's not. I will update. But if you are asking whether ObjectMapper is needed at all, there is a usage in the test to create the response body string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was asking about the FilterRegistrationBean

Copy link
Contributor

Choose a reason for hiding this comment

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

And it turns out that it is needed to silence warnings about e.g. missing X-SPINNAKER-USER, X-SPINNAKER-ACCOUNTS warnings...

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed some commits to silence those warnings, and some others.

@kirangodishala kirangodishala force-pushed the fix-delete-pipline-execution-error branch from 989ce8c to b529d7f Compare March 1, 2025 17:26
kirangodishala and others added 4 commits March 1, 2025 11:21
@kirangodishala @dbyron-sf
fix(web): fix the mismatches in the return types of apis in gate and …
174297e 
…orca for delete, resume, cancel and pause operations of a pipeline.
@dbyron-sf
refactor(web/test): mock DefaultProviderLookupService in PipelineServ…
dbf0d4a 
…iceTest

to remove errors from test output
@dbyron-sf
refactor(web/test): remove request id header from PipelineServiceTest
3f9df52 
since it's not needed
@dbyron-sf
refactor(web/test): add X-SPINNAKER-USER and X-SPINNAKER-ACCOUNTS hea…
073d67a 
…ders

to requests in PipelineServiceTest to silence warnings like:

2025-03-01 11:28:27.487  WARN 98848 --- [    Test worker] c.n.s.okhttp.OkHttp3MetricsInterceptor   : [] Request GET:http://localhost:52447/pipelines/my-pipeline-execution-id is missing [X-SPINNAKER-USER, X-SPINNAKER-ACCOUNTS] authentication headers and will be treated as anonymous.
Request from: com.netflix.spinnaker.okhttp.MetricsInterceptor.doIntercept(MetricsInterceptor.java:82)
	at com.netflix.spinnaker.okhttp.OkHttp3MetricsInterceptor.intercept(OkHttp3MetricsInterceptor.java:36)
	at com.netflix.spinnaker.okhttp.SpinnakerRequestHeaderInterceptor.intercept(SpinnakerRequestHeaderInterceptor.java:64)
	at com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory$ExecutorCallbackCall.execute(ErrorHandlingExecutorCallAdapterFactory.java:150)
	at com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall.executeCall(Retrofit2SyncCall.java:47)
	at com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall.execute(Retrofit2SyncCall.java:34)
	at com.netflix.spinnaker.gate.services.PipelineService.getPipeline(PipelineService.groovy:170)
	at com.netflix.spinnaker.gate.services.PipelineService$_setApplicationForExecution_closure2.doCall(PipelineService.groovy:222)
	at com.netflix.spinnaker.kork.core.RetrySupport.retry(RetrySupport.java:34)
	at com.netflix.spinnaker.kork.core.RetrySupport.retry(RetrySupport.java:27)
	at com.netflix.spinnaker.gate.services.PipelineService.setApplicationForExecution(PipelineService.groovy:222)
	at com.netflix.spinnaker.gate.services.PipelineService.deletePipeline(PipelineService.groovy:189)
	at com.netflix.spinnaker.gate.controllers.PipelineController.deletePipeline(PipelineController.groovy:279)
	at com.netflix.spinnaker.gate.service.PipelineServiceTest.invokeDeletePipelineExecution(PipelineServiceTest.java:135)
@dbyron-sf dbyron-sf force-pushed the fix-delete-pipline-execution-error branch from b529d7f to 073d67a Compare March 1, 2025 19:30
@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Mar 1, 2025
@mergify mergify bot added the auto merged label Mar 1, 2025
@mergify mergify bot merged commit 15465c9 into spinnaker:master Mar 1, 2025
5 checks passed
@dbyron-sf
Copy link
Contributor

@Mergifyio backport release-1.37.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 1, 2025 •
edited
Loading

backport release-1.37.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Mar 1, 2025
@kirangodishala @mergify
fix(web): fix the mismatches in the return types of APIs in gate and …
f881284 
…orca (#1883)

* test(web): add a test to demonstrate the mismatch in api response types of gate and orca when delete pipeline execution api is invoked

* fix(web): fix the mismatches in the return types of apis in gate and orca for delete, resume, cancel and pause operations of a pipeline.

* refactor(web/test): mock DefaultProviderLookupService in PipelineServiceTest

to remove errors from test output

* refactor(web/test): remove request id header from PipelineServiceTest

since it's not needed

* refactor(web/test): add X-SPINNAKER-USER and X-SPINNAKER-ACCOUNTS headers

to requests in PipelineServiceTest to silence warnings like:

2025-03-01 11:28:27.487  WARN 98848 --- [    Test worker] c.n.s.okhttp.OkHttp3MetricsInterceptor   : [] Request GET:http://localhost:52447/pipelines/my-pipeline-execution-id is missing [X-SPINNAKER-USER, X-SPINNAKER-ACCOUNTS] authentication headers and will be treated as anonymous.
Request from: com.netflix.spinnaker.okhttp.MetricsInterceptor.doIntercept(MetricsInterceptor.java:82)
	at com.netflix.spinnaker.okhttp.OkHttp3MetricsInterceptor.intercept(OkHttp3MetricsInterceptor.java:36)
	at com.netflix.spinnaker.okhttp.SpinnakerRequestHeaderInterceptor.intercept(SpinnakerRequestHeaderInterceptor.java:64)
	at com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory$ExecutorCallbackCall.execute(ErrorHandlingExecutorCallAdapterFactory.java:150)
	at com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall.executeCall(Retrofit2SyncCall.java:47)
	at com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall.execute(Retrofit2SyncCall.java:34)
	at com.netflix.spinnaker.gate.services.PipelineService.getPipeline(PipelineService.groovy:170)
	at com.netflix.spinnaker.gate.services.PipelineService$_setApplicationForExecution_closure2.doCall(PipelineService.groovy:222)
	at com.netflix.spinnaker.kork.core.RetrySupport.retry(RetrySupport.java:34)
	at com.netflix.spinnaker.kork.core.RetrySupport.retry(RetrySupport.java:27)
	at com.netflix.spinnaker.gate.services.PipelineService.setApplicationForExecution(PipelineService.groovy:222)
	at com.netflix.spinnaker.gate.services.PipelineService.deletePipeline(PipelineService.groovy:189)
	at com.netflix.spinnaker.gate.controllers.PipelineController.deletePipeline(PipelineController.groovy:279)
	at com.netflix.spinnaker.gate.service.PipelineServiceTest.invokeDeletePipelineExecution(PipelineServiceTest.java:135)

---------

Co-authored-by: David Byron <dbyron@salesforce.com>
(cherry picked from commit 15465c9)
@mergify mergify bot mentioned this pull request Mar 1, 2025
Merged
mergify bot added a commit that referenced this pull request Mar 1, 2025
@mergify @kirangodishala
fix(web): fix the mismatches in the return types of APIs in gate and …
de72fa6 
…orca (#1883) (#1884)

* test(web): add a test to demonstrate the mismatch in api response types of gate and orca when delete pipeline execution api is invoked

* fix(web): fix the mismatches in the return types of apis in gate and orca for delete, resume, cancel and pause operations of a pipeline.

* refactor(web/test): mock DefaultProviderLookupService in PipelineServiceTest

to remove errors from test output

* refactor(web/test): remove request id header from PipelineServiceTest

since it's not needed

* refactor(web/test): add X-SPINNAKER-USER and X-SPINNAKER-ACCOUNTS headers

to requests in PipelineServiceTest to silence warnings like:

2025-03-01 11:28:27.487  WARN 98848 --- [    Test worker] c.n.s.okhttp.OkHttp3MetricsInterceptor   : [] Request GET:http://localhost:52447/pipelines/my-pipeline-execution-id is missing [X-SPINNAKER-USER, X-SPINNAKER-ACCOUNTS] authentication headers and will be treated as anonymous.
Request from: com.netflix.spinnaker.okhttp.MetricsInterceptor.doIntercept(MetricsInterceptor.java:82)
	at com.netflix.spinnaker.okhttp.OkHttp3MetricsInterceptor.intercept(OkHttp3MetricsInterceptor.java:36)
	at com.netflix.spinnaker.okhttp.SpinnakerRequestHeaderInterceptor.intercept(SpinnakerRequestHeaderInterceptor.java:64)
	at com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory$ExecutorCallbackCall.execute(ErrorHandlingExecutorCallAdapterFactory.java:150)
	at com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall.executeCall(Retrofit2SyncCall.java:47)
	at com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall.execute(Retrofit2SyncCall.java:34)
	at com.netflix.spinnaker.gate.services.PipelineService.getPipeline(PipelineService.groovy:170)
	at com.netflix.spinnaker.gate.services.PipelineService$_setApplicationForExecution_closure2.doCall(PipelineService.groovy:222)
	at com.netflix.spinnaker.kork.core.RetrySupport.retry(RetrySupport.java:34)
	at com.netflix.spinnaker.kork.core.RetrySupport.retry(RetrySupport.java:27)
	at com.netflix.spinnaker.gate.services.PipelineService.setApplicationForExecution(PipelineService.groovy:222)
	at com.netflix.spinnaker.gate.services.PipelineService.deletePipeline(PipelineService.groovy:189)
	at com.netflix.spinnaker.gate.controllers.PipelineController.deletePipeline(PipelineController.groovy:279)
	at com.netflix.spinnaker.gate.service.PipelineServiceTest.invokeDeletePipelineExecution(PipelineServiceTest.java:135)

---------

Co-authored-by: David Byron <dbyron@salesforce.com>
(cherry picked from commit 15465c9)

Co-authored-by: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbyron-sf dbyron-sf dbyron-sf approved these changes

Assignees
No one assigned
Labels
auto merged ready to merge Approved and ready for merge target-release/1.38
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kirangodishala @dbyron-sf @spinnakerbot