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.

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

chore(exceptions): Fix up what exceptions are thrown #3835

Merged
merged 2 commits into from
Jul 26, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
* limitations under the License.
*/

package com.netflix.spinnaker.orca.clouddriver.exception
package com.netflix.spinnaker.orca.clouddriver.exception;

class PreconfiguredJobNotFoundException extends RuntimeException {
PreconfiguredJobNotFoundException(String jobKey) {
super("Could not find a stage named '$jobKey'")
import com.netflix.spinnaker.kork.exceptions.UserException;

public class PreconfiguredJobNotFoundException extends UserException {
public PreconfiguredJobNotFoundException(String jobKey) {
super("Could not find a stage named '" + jobKey + "'");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.cluster

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.frigga.Names
import com.netflix.spinnaker.kork.exceptions.ConfigurationException
import com.netflix.spinnaker.moniker.Moniker
import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
import com.netflix.spinnaker.orca.api.pipeline.TaskResult
import com.netflix.spinnaker.orca.clouddriver.OortService
import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException
import com.netflix.spinnaker.orca.clouddriver.tasks.AbstractCloudProviderAwareTask
import com.netflix.spinnaker.orca.pipeline.tasks.PreconditionTask
import groovy.transform.Canonical
Expand Down Expand Up @@ -82,7 +84,7 @@ class ClusterSizePreconditionTask extends AbstractCloudProviderAwareTask impleme
errors << 'Missing regions'
}
if (errors) {
throw new IllegalStateException("Invalid configuration " + errors.join(','))
throw new ConfigurationException("Invalid configuration " + errors.join(','))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new ConfigurationException("Invalid configuration " + errors.join(','))
throw new ConfigurationException("Invalid configuration " + errors.join(', '))

just to be consistent with line 136?

}
}
}
Expand Down Expand Up @@ -131,7 +133,7 @@ class ClusterSizePreconditionTask extends AbstractCloudProviderAwareTask impleme
}

if (failures) {
throw new IllegalStateException("Precondition check failed: ${failures.join(', ')}")
throw new PreconditionFailureException("Precondition check failed: ${failures.join(', ')}")
}

return TaskResult.SUCCEEDED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ class WaitForUpInstancesTask extends AbstractWaitingForInstancesTask {
if (System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(10) > Long.valueOf(taskStartTime.get())) {
// expectation is reconciliation has happened within 10 minutes and that the
// current server group capacity should be preferred
log.error(
log.warn(
"Short circuiting initial target capacity determination after 10 minutes (serverGroup: {}, executionId: {})",
"${cloudProvider}:${serverGroup.region}:${serverGroup.name}",
stage.execution.id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.cluster

import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException

import java.util.concurrent.atomic.AtomicInteger
import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
Expand Down Expand Up @@ -109,7 +111,7 @@ class ClusterSizePreconditionTaskSpec extends Specification {
then:
1 * oortService.getCluster('foo', 'test', 'foo', 'aws') >> response

thrown(IllegalStateException)
thrown(PreconditionFailureException)

where:
credentials = 'test'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright 2020 Netflix, 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.orca.exceptions;

import com.netflix.spinnaker.kork.exceptions.HasAdditionalAttributes;
import com.netflix.spinnaker.kork.exceptions.UserException;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;

public class PipelineTemplateValidationException extends UserException
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public class PipelineTemplateValidationException extends UserException
public final class PipelineTemplateValidationException extends UserException

implements HasAdditionalAttributes {
private final Collection errors;

public PipelineTemplateValidationException(Collection errors) {
this.errors = errors;
}

public PipelineTemplateValidationException(String message, Collection errors) {
super(message);
this.errors = errors;
}

public PipelineTemplateValidationException(String message, Throwable cause, Collection errors) {
super(message, cause);
this.errors = errors;
}

public PipelineTemplateValidationException(Throwable cause, Collection errors) {
super(cause);
this.errors = errors;
}

public PipelineTemplateValidationException(
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 all these constructors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

silly copy-paste, thank you! removing a bunch of them

String message,
Throwable cause,
boolean enableSuppression,
boolean writableStackTrace,
Collection errors) {
super(message, cause, enableSuppression, writableStackTrace);
this.errors = errors;
}

@Override
public Map<String, Object> getAdditionalAttributes() {
return errors != null && !errors.isEmpty()
? Collections.singletonMap("errors", errors)
: Collections.emptyMap();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright 2020 Netflix, 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.orca.exceptions;

import com.netflix.spinnaker.kork.exceptions.UserException;

public class PreconditionFailureException extends UserException {
public PreconditionFailureException(String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@

package com.netflix.spinnaker.orca.pipeline.tasks;

import com.netflix.spinnaker.kork.exceptions.ConfigurationException;
import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus;
import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException;
import javax.annotation.Nonnull;
import lombok.Getter;
import org.springframework.stereotype.Component;
Expand All @@ -38,12 +40,12 @@ public String getPreconditionType() {
String stageName = context.getStageName();
String assertedStatus = context.getStageStatus();
if (stageName == null) {
throw new IllegalArgumentException(
throw new ConfigurationException(
String.format(
"Stage name is required for preconditions of type %s.", getPreconditionType()));
}
if (assertedStatus == null) {
throw new IllegalArgumentException(
throw new ConfigurationException(
String.format(
"Stage status is required for preconditions of type %s.", getPreconditionType()));
}
Expand All @@ -53,13 +55,13 @@ public String getPreconditionType() {
.findFirst()
.orElseThrow(
() ->
new IllegalArgumentException(
new ConfigurationException(
String.format(
"Failed to find stage %s in execution. Please specify a valid stage name",
stageName)));
String actualStatus = foundStage.getStatus().toString();
if (!actualStatus.equals(assertedStatus)) {
throw new RuntimeException(
throw new PreconditionFailureException(
String.format(
"The status of stage %s was asserted to be %s, but was actually %s",
stageName, assertedStatus, actualStatus));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package com.netflix.spinnaker.orca.pipelinetemplate;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spinnaker.kork.web.exceptions.ValidationException;
import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
import com.netflix.spinnaker.orca.exceptions.PipelineTemplateValidationException;
import com.netflix.spinnaker.orca.extensionpoint.pipeline.ExecutionPreprocessor;
import com.netflix.spinnaker.orca.pipeline.expressions.PipelineExpressionEvaluator;
import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor;
Expand Down Expand Up @@ -50,7 +50,7 @@ public static Map<String, Object> planPipeline(

List<Map<String, Object>> pipelineErrors = (List<Map<String, Object>>) pipeline.get("errors");
if (pipelineErrors != null && !pipelineErrors.isEmpty()) {
throw new ValidationException("Pipeline template is invalid", pipelineErrors);
throw new PipelineTemplateValidationException("Pipeline template is invalid", pipelineErrors);
}

Map<String, Object> augmentedContext = new HashMap<>();
Expand All @@ -72,7 +72,7 @@ public static Map<String, Object> planPipeline(
.collect(Collectors.toList());

if (failedTemplateVars.size() > 0) {
throw new ValidationException(
throw new PipelineTemplateValidationException(
"Missing template variable values for the following variables: %s", failedTemplateVars);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.netflix.spinnaker.orca.pipeline.tasks

import com.netflix.spinnaker.kork.exceptions.ConfigurationException
import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException
import spock.lang.Specification
import spock.lang.Unroll
import static com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.SUCCEEDED
Expand Down Expand Up @@ -79,7 +81,7 @@ class StageStatusPreconditionTaskSpec extends Specification {
task.execute(stage)

then:
thrown(IllegalArgumentException)
thrown(ConfigurationException)

where:
stageName | stageStatus
Expand Down Expand Up @@ -113,7 +115,7 @@ class StageStatusPreconditionTaskSpec extends Specification {
task.execute(stage)

then:
thrown(RuntimeException)
thrown(PreconditionFailureException)

where:
stageName | stageStatus
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ package com.netflix.spinnaker.orca.front50
import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spectator.api.Id
import com.netflix.spectator.api.Registry
import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
import com.netflix.spinnaker.kork.web.exceptions.ValidationException
import com.netflix.spinnaker.kork.exceptions.ConfigurationException
import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
import com.netflix.spinnaker.orca.api.pipeline.models.Trigger
import com.netflix.spinnaker.orca.exceptions.PipelineTemplateValidationException
import com.netflix.spinnaker.orca.extensionpoint.pipeline.ExecutionPreprocessor
import com.netflix.spinnaker.orca.pipeline.ExecutionLauncher
import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils
Expand Down Expand Up @@ -74,7 +74,7 @@ class DependentPipelineStarter implements ApplicationContextAware {
def json = objectMapper.writeValueAsString(pipelineConfig)

if (pipelineConfig.disabled) {
throw new InvalidRequestException("Pipeline '${pipelineConfig.name}' is disabled and cannot be triggered")
throw new ConfigurationException("Pipeline '${pipelineConfig.name}' is disabled and cannot be triggered")
}

log.info('triggering dependent pipeline {}:{}', pipelineConfig.id, json)
Expand Down Expand Up @@ -134,7 +134,7 @@ class DependentPipelineStarter implements ApplicationContextAware {
}

if (pipelineConfig.errors != null) {
throw new ValidationException("Pipeline template is invalid", pipelineConfig.errors as List<Map<String, Object>>)
throw new PipelineTemplateValidationException("Pipeline template is invalid", pipelineConfig.errors as List<Map<String, Object>>)
}

// Process the raw trigger to resolve any expressions before converting it to a Trigger object, which will not be
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.spinnaker.orca.front50.tasks

import com.netflix.spinnaker.kork.exceptions.ConfigurationException
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
import com.netflix.spinnaker.orca.api.pipeline.Task
import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
Expand Down Expand Up @@ -70,11 +71,11 @@ class StartPipelineTask implements Task {
Map<String, Object> pipelineConfig = pipelines.find { it.id == pipelineId }

if (!pipelineConfig) {
throw new IllegalArgumentException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} cannot be located (${pipelineId})")
throw new ConfigurationException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} cannot be located (${pipelineId})")
}

if (pipelineConfig.getOrDefault("disabled", false)) {
throw new IllegalArgumentException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} is disabled")
throw new ConfigurationException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} is disabled")
}

if (V2Util.isV2Pipeline(pipelineConfig)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@ import com.netflix.spinnaker.fiat.model.UserPermission
import com.netflix.spinnaker.fiat.model.resources.Role
import com.netflix.spinnaker.fiat.shared.FiatService
import com.netflix.spinnaker.fiat.shared.FiatStatus
import com.netflix.spinnaker.kork.exceptions.ConfigurationException
import com.netflix.spinnaker.kork.exceptions.SpinnakerException
import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
import com.netflix.spinnaker.kork.web.exceptions.ValidationException
import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
import com.netflix.spinnaker.orca.clouddriver.service.JobService
import com.netflix.spinnaker.orca.exceptions.OperationFailedException
import com.netflix.spinnaker.orca.exceptions.PipelineTemplateValidationException
import com.netflix.spinnaker.orca.extensionpoint.pipeline.ExecutionPreprocessor
import com.netflix.spinnaker.orca.front50.Front50Service
import com.netflix.spinnaker.orca.front50.PipelineModelMutator
Expand Down Expand Up @@ -240,7 +242,7 @@ class OperationsController {
}

if (pipeline.disabled) {
throw new InvalidRequestException("Pipeline is disabled and cannot be started.")
throw new ConfigurationException("Pipeline is disabled and cannot be started.")
}

def linear = pipeline.stages.every { it.refId == null }
Expand All @@ -249,7 +251,7 @@ class OperationsController {
}

if (pipeline.errors != null) {
throw new ValidationException("Pipeline template is invalid", pipeline.errors as List<Map<String, Object>>)
throw new PipelineTemplateValidationException("Pipeline template is invalid", pipeline.errors as List<Map<String, Object>>)
}
return pipeline
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ import com.netflix.spinnaker.fiat.model.resources.Account
import com.netflix.spinnaker.fiat.model.resources.Role
import com.netflix.spinnaker.fiat.shared.FiatService
import com.netflix.spinnaker.fiat.shared.FiatStatus
import com.netflix.spinnaker.kork.exceptions.ConfigurationException
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType
import com.netflix.spinnaker.orca.api.preconfigured.jobs.TitusPreconfiguredJobProperties
import com.netflix.spinnaker.orca.clouddriver.service.JobService
import com.netflix.spinnaker.orca.exceptions.PipelineTemplateValidationException
import com.netflix.spinnaker.orca.front50.Front50Service

import javax.servlet.http.HttpServletResponse
import com.netflix.spinnaker.kork.common.Header
import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
import com.netflix.spinnaker.kork.web.exceptions.ValidationException
import com.netflix.spinnaker.orca.igor.BuildService
import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper
import com.netflix.spinnaker.orca.pipeline.ExecutionLauncher
Expand Down Expand Up @@ -567,7 +567,7 @@ class OperationsControllerSpec extends Specification {
controller.orchestrate(pipelineConfig, response)

then:
thrown(InvalidRequestException)
thrown(PipelineTemplateValidationException)
1 * pipelineTemplateService.retrievePipelineOrNewestExecution("12345", null) >> {
throw new ExecutionNotFoundException("Not found")
}
Expand All @@ -594,7 +594,7 @@ class OperationsControllerSpec extends Specification {
controller.orchestrate(pipelineConfig, response)

then:
thrown(ValidationException)
thrown(PipelineTemplateValidationException)
0 * executionLauncher.start(*_)
}

Expand Down