-
Notifications
You must be signed in to change notification settings - Fork 806
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(pipeline executions/orca) : Manual Judgement Navigation Enhancement Backend API Implementation #4132
base: master
Are you sure you want to change the base?
Conversation
…ent Backend API Implementation. Enhanced ManualJudgmentStage.groovy to To check if there are any parent executions waiting for manual judgment stage. If yes, fetch all the parent executions and set the waiting manual judgment stage's(this) execution and application name as leafnodeExecutionId and leafnodeApplicationName in parent's execution context others attribute and save it to the underlying storage. Once the user enters the manual judgment input(Continue/Stop), it deletes the leafnodeExecutionId and leafnodeApplicationName from all the parent executions. If no, continue the execution as usual. Enhanced RedisExecutionRepository.java to Save the leafnode attributes to the redis storage. Enhanced SqlExecutionRepository.java to Save the leafnode attributes to the sql storage.
This is part of the deck PR... Deck PR is merged and scheduled to release for 1.27... Deck governance PR is also merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments -- this isn't a feature we're going to be using in our distribution but would appreciate it going in with as minimal a change as possible to the core
objects.
@@ -86,6 +86,11 @@ | |||
|
|||
void setContext(@Nonnull Map<String, Object> context); | |||
|
|||
@Nonnull | |||
Map<String, Object> getOthers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a very strangely named method, with no comments on what it's used for.
@@ -73,14 +75,20 @@ class ManualJudgmentStage implements StageDefinitionBuilder, AuthenticatedStage | |||
final long backoffPeriod = 15000 | |||
final long timeout = TimeUnit.DAYS.toMillis(3) | |||
|
|||
@Value('${spinnaker.manual-judgment-navigation:false}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there's a bit of prior art under the stages
prefix.
Perhaps, stages.manual-judgment.navigation
.
@@ -89,14 +97,24 @@ class ManualJudgmentStage implements StageDefinitionBuilder, AuthenticatedStage | |||
String notificationState | |||
ExecutionStatus executionStatus | |||
|
|||
if (manualJudgmentNavigation) { | |||
checkForAnyParentExecutions(stage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming of this method implies no side effects, I'd suggest a change of name.
*/ | ||
void checkForAnyParentExecutions(StageExecution stage) { | ||
|
||
def status = stage?.execution?.status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what situation is stage
null?
PipelineExecution parentExecution = trigger?.parentExecution | ||
parentExecution = executionRepository.retrieve(ExecutionType.PIPELINE, parentExecution.id) | ||
parentExecution.getStages().each { | ||
if (("pipeline").equals(it.getType()) && (ExecutionStatus.RUNNING.equals(it.getStatus()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably a constant for pipeline
somewhere in the PipelineStage
?
@@ -146,6 +146,16 @@ class SqlExecutionRepository( | |||
storeStage(stage) | |||
} | |||
|
|||
override fun updateStageOthers(stage: StageExecution) { | |||
storeStage(stage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just boiling down to saving the entire stage ... why bother introducing "others" (which is likely going to be confusing) instead of just storing in the stage relevant stage context.
feat(pipeline executions/orca) : Manual Judgement Navigation Enhancement Backend API Implementation
Enhanced ManualJudgmentStage.groovy to
To check if there are any parent executions waiting for manual judgment stage.
If yes, fetch all the parent executions and set the waiting manual judgment stage's(this)
execution and application name as leafnodeExecutionId and leafnodeApplicationName in parent's
execution context others attribute and save it to the underlying storage.
Once the user enters the manual judgment input(Continue/Stop), it deletes the
leafnodeExecutionId and leafnodeApplicationName from all the parent executions.
If no, continue the execution as usual.
Enhanced RedisExecutionRepository.java to
Save the leafnode attributes to the redis storage.
Enhanced SqlExecutionRepository.java to
Save the leafnode attributes to the sql storage.
Enhanced ManualJudgmentStageSpec.groovy to
Modified the testcases as per the requirement.
Manual Judgment Backend API Implementation.pdf