feat(retrofit): move url attribute from SpinnakerHttpException to SpinnakerServerException #1110

dbyron-sf · 2023-10-28T23:31:20Z

so it's available to exception handling code that currently has access to the url via RetrofitError.

to use fluent assertions and other small tweaks

to prepare to move url to SpinnakerServerException

…2ErrorHandleTest testRetrofitSimpleSpinnakerServerException actually generates a SpinnakerNetworkException and so it's effectively the same test as testRetrofitSimpleSpinnakerNetworkException.

…innakerNetworkException

…nnakerServerException so it's available to exception handling code that currently has access to the url via RetrofitError.

xibz

Overall looks good! One minor design consideration, but overall I like the approach

xibz · 2023-10-28T23:49:18Z

.../main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandler.java

@@ -59,11 +59,11 @@ public Throwable handleError(RetrofitError e) {
        }
        return retval;
      case NETWORK:
-        return new SpinnakerNetworkException(e.getMessage(), e.getCause());
+        return new SpinnakerNetworkException(e);


This becomes easy to change too

Suggested change

return new SpinnakerNetworkException(e);

return new SpinnakerNetworkException(e.getMessage(), e.getCause(), e.getUrl());

xibz · 2023-10-28T23:49:33Z

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java

  }

-  public SpinnakerServerException() {}
+  public SpinnakerServerException(String message, Throwable cause, String url) {


I think with this new method signature, we can probably get rid of all the retrofit constructors tbh.

This will at least clean up the other exceptions and only require updating the one constructor in each other exception. This also keeps the SpinnakerServerException pretty agnostic to retrofit, which may be a better approach for now since we are only interested in the URL at this point. Further, if we do end up with more fields from retrofit in this particular class, we can revisit. However, let's keep it simple and just add the one field :)

xibz · 2023-10-28T23:49:51Z

.../main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandler.java

      case CONVERSION:
-        return new SpinnakerConversionException(e.getMessage(), e.getCause());
+        return new SpinnakerConversionException(e);


Suggested change

return new SpinnakerConversionException(e);

return new SpinnakerConversionException(e.getMessage(), e.getCause(), e.getUrl());

xibz · 2023-10-28T23:50:09Z

.../main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerRetrofitErrorHandler.java

      default:
-        return new SpinnakerServerException(e.getMessage(), e.getCause());
+        return new SpinnakerServerException(e);


Suggested change

return new SpinnakerServerException(e);

return new SpinnakerServerException(e.getMessage(), e.getCause(), e.getUrl());

xibz · 2023-10-28T23:50:44Z

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java

-    super(message, cause);
+  private final String url;
+
+  public SpinnakerServerException(RetrofitError e) {


Then these go away and allows for only the SpinnakerHttpException to utilize all the retrofit fields

… Request instead of a url, to pave the way for storing other attributes of requests (e.g. the http request method)

…e new SpinnakerConversionException constructors

xibz · 2023-10-29T22:38:40Z

...c/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerConversionException.java

  public SpinnakerConversionException(RetrofitError e) {
    super(e);
+    setRetryable(false);


Oof, that sucks we need to add the false everywhere. May be nice to have a implements NonRetryableException or something like that, but that is separate from this PR

xibz

@dbyron-sf Thank you for the improvements! The javadocs are fantastic :) and really helped me understand the use cases.

I had one very minor nit comment, but this change looks really good :)

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java

…etworkException constructor changes from spinnaker/kork#1110

…or changes from spinnaker/kork#1110

* chore(dependencies): Autobump korkVersion * fix(manifests/test): keep up with SpinnakerNetworkException constructor changes from spinnaker/kork#1110 --------- Co-authored-by: root <root@2f91e8016c3c> Co-authored-by: David Byron <dbyron@salesforce.com>

* chore(dependencies): Autobump korkVersion * fix(retrofit/test): keep up with SpinnakerServerException, SpinnakerNetworkException constructor changes from spinnaker/kork#1110 --------- Co-authored-by: root <root@2f91e8016c3c> Co-authored-by: David Byron <dbyron@salesforce.com>

…OfProject The jenkinsClient inside JenkinsService comes from JenkinsConfig, wihch, as of spinnaker#1130, uses SpinnakerRetrofitErrorHandler. Therefore, the getBuilds calls inside the try/catch block inside processBuildsOfProject throws a SpinnakerServerException. spinnaker#1130 updated the 'should continue processing other builds from a master even if one or more build fetches fail' test to throw a SpinnakerServerException with a RetrofitError as its cause instead of a RuntimeException with a RetrofitError as its cause. With spinnaker/kork#1110, the SpinnakerServerException no longer has a RetrofitError as its cause. It has the "mock root cause" exception as its cause. But, because spinnaker/kork#1110 adds a url attribute to SpinnakerServerException, the cause is no longer necessary for including a url in the "Error communicating with jenkins" log message, and this commit updates the logic accordingly.

* chore(dependencies): Autobump korkVersion * fix(web): fix exception handling in JenkinsBuildMonitor.processBuildsOfProject The jenkinsClient inside JenkinsService comes from JenkinsConfig, wihch, as of #1130, uses SpinnakerRetrofitErrorHandler. Therefore, the getBuilds calls inside the try/catch block inside processBuildsOfProject throws a SpinnakerServerException. #1130 updated the 'should continue processing other builds from a master even if one or more build fetches fail' test to throw a SpinnakerServerException with a RetrofitError as its cause instead of a RuntimeException with a RetrofitError as its cause. With spinnaker/kork#1110, the SpinnakerServerException no longer has a RetrofitError as its cause. It has the "mock root cause" exception as its cause. But, because spinnaker/kork#1110 adds a url attribute to SpinnakerServerException, the cause is no longer necessary for including a url in the "Error communicating with jenkins" log message, and this commit updates the logic accordingly. * refactor(web/test): clean up exception mocking in JenkinsBuildMonitorSpec remove unused runtimeException and rename retrofitEx since it's no longer a retrofitEx --------- Co-authored-by: root <root@2f91e8016c3c> Co-authored-by: David Byron <dbyron@salesforce.com>

…nges from spinnaker/kork#1110

* chore(dependencies): Autobump korkVersion * fix(aws/test): keep up with SpinnakerNetworkException constructor changes from spinnaker/kork#1110 --------- Co-authored-by: root <root@2f91e8016c3c> Co-authored-by: David Byron <dbyron@salesforce.com>

…anges from spinnaker/kork#1110

* chore(dependencies): Autobump orcaVersion * fix(core/test): keep up with SpinnakerNetworkException constructor changes from spinnaker/kork#1110 --------- Co-authored-by: root <root@37e47cedb6bb> Co-authored-by: David Byron <dbyron@salesforce.com>

* chore(dependencies): Autobump korkVersion * fix(retrofit/test): keep up with SpinnakerServerException, SpinnakerNetworkException constructor changes from spinnaker/kork#1110 --------- Co-authored-by: root <root@2f91e8016c3c> Co-authored-by: David Byron <dbyron@salesforce.com> feat(retrofit2): orca-clouddriver: InstanceService - Eliminated the usage of RetrofitError in the catch blocks and added SpinnakerRetrofitErrorHandler in the retrofit config

dbyron-sf added 5 commits October 28, 2023 16:20

refactor(retrofit/test): cleanup tests in kork-retrofit

7c9b572

to use fluent assertions and other small tweaks

test(retrofit): add assertions on SpinnakerHttpException's url

b212235

to prepare to move url to SpinnakerServerException

refactor(retrofit/test): remove duplicate test from SpinnakerRetrofit…

660c79b

…2ErrorHandleTest testRetrofitSimpleSpinnakerServerException actually generates a SpinnakerNetworkException and so it's effectively the same test as testRetrofitSimpleSpinnakerNetworkException.

test(retrofit): test behavior when a retrofit endpoint generates a Sp…

2fdbee3

…innakerNetworkException

feat(retrofit): move url attribute from SpinnakerHttpException to Spi…

8eac81a

…nnakerServerException so it's available to exception handling code that currently has access to the url via RetrofitError.

xibz suggested changes Oct 28, 2023

View reviewed changes

dbyron-sf added 2 commits October 29, 2023 13:05

refactor(retrofit): change Spinnaker*Exception constructors to take a…

669d649

… Request instead of a url, to pave the way for storing other attributes of requests (e.g. the http request method)

fix(retrofit): add javadoc, and properly set retryable to false in th…

f4bb645

…e new SpinnakerConversionException constructors

xibz reviewed Oct 29, 2023

View reviewed changes

xibz approved these changes Oct 29, 2023

View reviewed changes

xibz reviewed Oct 29, 2023

View reviewed changes

...t/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java Show resolved Hide resolved

refactor(retrofit): use @getter for url in SpinnakerServerException

64f7673

xibz approved these changes Oct 29, 2023

View reviewed changes

dbyron-sf added the ready to merge Approved and ready for merge label Oct 29, 2023

mergify bot added the auto merged label Oct 29, 2023

mergify bot merged commit c90aabb into spinnaker:master Oct 29, 2023
5 checks passed

dbyron-sf deleted the move-url-to-SpinnakerServerException branch October 29, 2023 23:16

spinnakerbot added the target-release/1.33 label Oct 29, 2023

dbyron-sf added a commit to spinnakerbot/orca that referenced this pull request Oct 30, 2023

fix(retrofit/test): keep up with SpinnakerServerException, SpinnakerN…

1c51e2d

…etworkException constructor changes from spinnaker/kork#1110

dbyron-sf added a commit to spinnakerbot/rosco that referenced this pull request Oct 30, 2023

fix(manifests/test): keep up with SpinnakerNetworkException construct…

577c8a1

…or changes from spinnaker/kork#1110

dbyron-sf added a commit to spinnakerbot/clouddriver that referenced this pull request Oct 30, 2023

fix(aws/test): keep up with SpinnakerNetworkException constructor cha…

85f7d73

…nges from spinnaker/kork#1110

dbyron-sf added a commit to spinnakerbot/kayenta that referenced this pull request Oct 30, 2023

fix(core/test): keep up with SpinnakerNetworkException constructor ch…

b919353

…anges from spinnaker/kork#1110

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(retrofit): move url attribute from SpinnakerHttpException to SpinnakerServerException #1110

feat(retrofit): move url attribute from SpinnakerHttpException to SpinnakerServerException #1110

dbyron-sf commented Oct 28, 2023

xibz left a comment

xibz Oct 28, 2023

xibz Oct 28, 2023

xibz Oct 28, 2023

xibz Oct 28, 2023

xibz Oct 28, 2023

xibz Oct 29, 2023

xibz left a comment

	return new SpinnakerNetworkException(e);
	return new SpinnakerNetworkException(e.getMessage(), e.getCause(), e.getUrl());

	return new SpinnakerConversionException(e);
	return new SpinnakerConversionException(e.getMessage(), e.getCause(), e.getUrl());

	return new SpinnakerServerException(e);
	return new SpinnakerServerException(e.getMessage(), e.getCause(), e.getUrl());

feat(retrofit): move url attribute from SpinnakerHttpException to SpinnakerServerException #1110

feat(retrofit): move url attribute from SpinnakerHttpException to SpinnakerServerException #1110

Conversation

dbyron-sf commented Oct 28, 2023

xibz left a comment

Choose a reason for hiding this comment

xibz Oct 28, 2023

Choose a reason for hiding this comment

xibz Oct 28, 2023

Choose a reason for hiding this comment

xibz Oct 28, 2023

Choose a reason for hiding this comment

xibz Oct 28, 2023

Choose a reason for hiding this comment

xibz Oct 28, 2023

Choose a reason for hiding this comment

xibz Oct 29, 2023

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment