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

feat(core): Re-introduce executionEngine flag, add ExecutionEngineRunner that selects an underlying ExecutionRunner based on the specified ExecutionEngine #3984

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonsie
Copy link
Contributor

@jonsie jonsie commented Oct 30, 2020

Adds back the flag that was removed here: https://github.com/spinnaker/orca/pull/1917/files

Also moves the ExecutionRunner interface to orca-api and introduces a composite ExecutionRunner class ExecutionEngineRunner.

This allows us to experiment with a v4 execution engine.

@jonsie jonsie force-pushed the execution-engine branch 2 times, most recently from 0646923 to 7c67278 Compare October 30, 2020 04:13
…ner that selects an underlying ExecutionRunner based on the specified ExecutionEngine
Copy link
Contributor

@srekapalli srekapalli left a comment

Choose a reason for hiding this comment

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

LGTM

import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
import com.netflix.spinnaker.orca.pipeline.ExecutionRunner
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionEngine.v3
import com.netflix.spinnaker.q.Queue
import com.netflix.spinnaker.security.AuthenticatedRequest
import org.springframework.stereotype.Component

@Component
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious. We drive the decision of what runner to use based on the annotation being present ..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I could change that if we don't like it but it seemed to make sense to me. I didn't want to add a field to ExecutionRunner since the composite ExecutionEngineRunner takes a list of ExecutionRunner objects.


import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

/** The touch point into running a {@link PipelineExecution}. */
Copy link
Member

Choose a reason for hiding this comment

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

Can we get some docs on the method as well?

@@ -0,0 +1,18 @@
package com.netflix.spinnaker.orca.api.pipeline.models;

public enum ExecutionEngine {
Copy link
Member

Choose a reason for hiding this comment

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

Would like docs on this.

Comment on lines +70 to +71
new UnsupportedOperationException(
"No execution engine runner found!")));
Copy link
Member

Choose a reason for hiding this comment

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

Use a subtype of SystemException?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants