-
Notifications
You must be signed in to change notification settings - Fork 903
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_template): Better support for templated pipelines with dynamic sources - Take 2 #4365
feat(pipeline_template): Better support for templated pipelines with dynamic sources - Take 2 #4365
Conversation
6e548af
to
d951e61
Compare
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.
One really minor thing; LGTM otherwise
<li role="presentation" ng-class="{selected: $ctrl.mode === 'pipeline'}"><a ng-click="$ctrl.mode = 'pipeline'">Configuration</a></li> | ||
<li role="presentation" ng-class="{selected: $ctrl.mode === 'renderedPipeline'}"><a ng-click="$ctrl.mode = 'renderedPipeline'">Rendered pipeline</a></li> | ||
</ul> | ||
<div class="modal-body flex-fill" ng-if="$ctrl.mode == 'pipeline'"> |
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.
use ===
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.
Fixed 👍
d951e61
to
e190ad7
Compare
<span class="glyphicon glyphicon small glyphicon-alert"></span> This template has a dynamic source. The | ||
configuration cannot be rendered before the pipeline has executed at least once. | ||
</div> | ||
<div class="band band-warning" ng-if="templateError && pipelineExecutions.length && !pipeline.isNew"> |
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 still swallows errors if you're not using dynamic sources. I have a config that doesn't render a pipeline properly - with these changes, when I load the pipeline config screen, there's no indication that anything's wrong.
The edit config modal should open if there's a problem rendering the template - this behavior isn't perfect, but it gives the user a chance to update their config (e.g., in the case they're missing a variable), and then view the rendering error if it still exists.
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.
Better? 98eb73c
e190ad7
to
98eb73c
Compare
…dynamic sources * Add "Rendered pipeline" tab to "Edit as JSON" modal for templated pipelines * Don't display error message when editing templated pipelines with dynamic source - real fix in Orca * Render execution graph when editing templated pipelines with dynamic source - real fix in Orca * Display warning in Deck if configuration can't be rendered because no executions have been run * Display information about which execution used for rendering * Let the user select which configuration that's used for rendering Depends on spinnaker/orca#1718 and spinnaker/gate#471
98eb73c
to
a62e114
Compare
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.
Awesome, thank you. LGTM.
…eline_templates_take_2
This is the second iteration of #4288, which had to be rolled back in #4362.
It was an oversight by me, I was not really aware that the pipeline executions sometimes had both a build number and a pipeline parent. This is fixed.
Original PR: