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(pipelines): Pass 'staleCheck' through from savePipeline context to front50 #3841

Merged

Conversation

christopherthielen
Copy link
Contributor

No description provided.

@christopherthielen
Copy link
Contributor Author

christopherthielen commented Aug 4, 2020

need to update tests...

@@ -77,6 +77,8 @@ public TaskResult execute(StageExecution stage) {
final Boolean isSavingMultiplePipelines =
(Boolean)
Optional.ofNullable(stage.getContext().get("isSavingMultiplePipelines")).orElse(false);
final Boolean staleCheck =
(Boolean) Optional.ofNullable(stage.getContext().get("staleCheck")).orElse(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is sprinkled here across the whole task (and orca...) but really this should be

Suggested change
(Boolean) Optional.ofNullable(stage.getContext().get("staleCheck")).orElse(false);
(Boolean) Optional.ofNullable(stage.getContext().getCurrentOnly("staleCheck")).orElse(false);

otherwise if there is an evalVars upstream that has a variable stageCheck we might end up using that if none is present on this stage context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, in StageExecutionImpl:

  /** The context driving this stage. Provides inputs necessary to component steps */
  private Map<String, Object> context = new StageContext(this);

... feels pretty nasty to cast this to a StageContext
Since StageContext implements Map, would it be reasonable to change StageExecution to return a StageContext?

  /** TODO(rz): Try to use StageContext instead? */
  @Nonnull
  Map<String, Object> getContext();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

welp, StageContext isn't available in orca-api so we'd have to extract an interface for it to do ☝️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we are casting in one spot already, the pipeline stage

StageContext context = (StageContext) stage.getContext();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after discussing with mark offline, we're leaving it as-is

@christopherthielen christopherthielen added the ready to merge Approved and ready for merge label Aug 11, 2020
@mergify mergify bot added the auto merged Merged automatically by a bot label Aug 11, 2020
mergify bot added a commit to spinnaker/gate that referenced this pull request Aug 11, 2020
Related to spinnaker/orca#3841

Co-authored-by: spinnaker <spinnaker@netflix.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@mergify mergify bot merged commit da35b8e into spinnaker:master Aug 11, 2020
@christopherthielen christopherthielen deleted the save-pipeline-stale-check branch November 9, 2020 23:16
KathrynLewis pushed a commit to KathrynLewis/orca that referenced this pull request Jan 31, 2021
…to front50 (spinnaker#3841)

* feat(pipelines): Pass 'staleCheck' through from savePipeline context to front50

Related to spinnaker/front50#915

* test: update SavePipelineTaskSpec

Co-authored-by: spinnaker <spinnaker@netflix.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.22
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants