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

fix(error handling): mark executions failed #638

Merged
merged 3 commits into from
Sep 4, 2019

Conversation

marchello2000
Copy link
Contributor

We need a way to indicate failures in materializing a pipeline.
If echo fails to materialize a pipeline during a trigger the failure needs to be recorded
in orca otherwise, the failure appears "silent" to the user because the UI is unable to
show any errors unless they are associated with an execution.
This is especially bad if, say, there is a typo in the properties file in the jenkins trigger
Starting the pipeline appears to do nothing and the user is confused.

This change captures errors during artifact resolution and sends them to orca to capture for the user

NOTE: depends on related orca PR: spinnaker/orca#3126

We need a way to indicate failures in materializing a pipeline.
If `echo` fails to materialize a pipeline during a trigger the failure needs to be recorded
in `orca` otherwise the failure appears "silent" to the user because the UI is unable to
show any errors unless they are associated with an execution.
This is especially bad if, say, there is a typo in the properties file in the jenkins trigger
Starting the pipeline appears to do nothing and the user is confused.

This change captures errors during artifact resolution and sends them to `orca` to capture for the user
>NOTE: depends on related `orca` PR: spinnaker/orca#3126
@@ -74,12 +74,17 @@ private void triggerMatchingPipelines(T event) {
try {
List<Pipeline> matchingPipelines = eventHandler.getMatchingPipelines(event, pipelineCache);
matchingPipelines.stream()
.filter(p -> p.getErrorMessage() == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on whether we should also allow the empty string through this filter? (And exclude it from the filter below?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was contemplating that myself and don't recall why i picked one over the other :)
but it's prob safer to isNullOrEmpty()it

@marchello2000 marchello2000 merged commit 7e33063 into spinnaker:master Sep 4, 2019
@marchello2000 marchello2000 deleted the mark/spin-4961 branch September 4, 2019 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants