From 6b181d6a6c5799040f14f1ea851e08f0fa74c791 Mon Sep 17 00:00:00 2001 From: Eric Zimanyi Date: Mon, 30 Sep 2019 10:12:00 -0400 Subject: [PATCH] fix(kubernetes): Fix merge strategy field (#7455) (#7462) * fix(kubernetes): Properly dirty patch stage on resource update We're not currently not marking the stage dirty upon chaning the resource to be selected, which makes it impossible to save (without making some other change or waiting for a digest cycle to happen for another reason). * fix(kubernetes): Fix manifest source defaulting in patch manifest When creating a new patch manifest stage, we default the 'Manifest Source' selector to 'Text' but the 'Manifest' text box is not visible; the user needs to choose 'Artifact' then 'Text' again to make the text box visible. * fix(kubernetes): Fix merge strategy field We've been incorrectly setting the merge strategy on options.strategy instead of options.mergeStrategy since 1.15. Start writing to the correct field, and handle updating any stages affected by the bug by deleting the value of 'strategy' and copying it over to mergeStrategy. * fix(kubernetes): Fix defaulting of strategy field The prior commit was incorrect; delete does not return the deleted value. * fix(kubernetes): Fix display of strategy field --- .../PatchManifestStageConfig.tsx | 58 +++++++++---------- .../patchManifest/PatchManifestStageForm.tsx | 11 ++-- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageConfig.tsx b/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageConfig.tsx index 85e65cdde30..7d2f667bf27 100644 --- a/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageConfig.tsx +++ b/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageConfig.tsx @@ -1,57 +1,53 @@ import * as React from 'react'; import { defaults } from 'lodash'; -import { Observable, Subject } from 'rxjs'; import { FormikStageConfig, IStage, IStageConfigProps } from '@spinnaker/core'; -import { KubernetesManifestCommandBuilder } from 'kubernetes/v2/manifest/manifestCommandBuilder.service'; import { PatchManifestStageForm } from './PatchManifestStageForm'; export class PatchManifestStageConfig extends React.Component { private readonly stage: IStage; - private destroy$ = new Subject(); public constructor(props: IStageConfigProps) { super(props); + if (props.stage.isNew) { + defaults(props.stage, { + source: 'text', + options: { + record: true, + mergeStrategy: 'strategic', + }, + cloudProvider: 'kubernetes', + }); + } + + // There was a bug introduced in Spinnaker 1.15 where we were incorrectly + // storing the merge strategy on a field called 'strategy' instead of on + // 'mergeStrategy'. In order to auto-fix pipelines affected by that bug, + // delete any value in 'strategy'. If 'mergeStrategy' is empty, set it to + // the value we deleted from 'strategy'. + defaults(props.stage.options, { + mergeStrategy: props.stage.options.strategy, + }); + delete props.stage.options.strategy; // Intentionally initializing the stage config only once in the constructor // The stage config is then completely owned within FormikStageConfig's Formik state this.stage = props.stage; } - public componentDidMount(): void { - Observable.fromPromise( - KubernetesManifestCommandBuilder.buildNewManifestCommand( - this.props.application, - this.stage.patchBody, - this.stage.moniker, - ), - ) - .takeUntil(this.destroy$) - .subscribe(builtCommand => { - if (this.stage.isNew) { - defaults(this.stage, { - account: builtCommand.command.account, - manifestArtifactAccount: 'embedded-artifact', - patchBody: builtCommand.command.manifest, - source: 'text', - options: { - record: true, - strategy: 'strategic', - }, - location: '', - cloudProvider: 'kubernetes', - }); - } - }); - } - public render() { return ( } + render={props => ( + + )} /> ); } diff --git a/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageForm.tsx b/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageForm.tsx index 80392456f68..a8e03450fff 100644 --- a/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageForm.tsx +++ b/app/scripts/modules/kubernetes/src/v2/pipelines/stages/patchManifest/PatchManifestStageForm.tsx @@ -22,6 +22,7 @@ import { SelectorMode } from 'kubernetes/v2/manifest/selector/IManifestSelector' import { PatchManifestOptionsForm } from './PatchManifestOptionsForm'; interface IPatchManifestStageConfigFormProps { + stageFieldUpdated: () => void; updatePipeline: (pipeline: IPipeline) => void; } @@ -88,7 +89,9 @@ export class PatchManifestStageForm extends React.Component< this.props.formik.setFieldValue('patchBody', manifests[0]); }; - private onManifestSelectorChange = (): void => {}; + private onManifestSelectorChange = (): void => { + this.props.stageFieldUpdated(); + }; private getSourceOptions = (): Array> => { return map([this.textSource, this.artifactSource], option => ({ @@ -114,8 +117,8 @@ export class PatchManifestStageForm extends React.Component< this.props.formik.setFieldValue('source', e.target.value)} - value={stage.source || 'text'} inline={true} + value={stage.source} /> {stage.source === this.textSource && ( @@ -155,8 +158,8 @@ export class PatchManifestStageForm extends React.Component<

Patch Options

this.props.formik.setFieldValue('options.strategy', strategy)} + strategy={!!stage.options && stage.options.mergeStrategy} + onStrategyChange={(strategy: string) => this.props.formik.setFieldValue('options.mergeStrategy', strategy)} record={!!stage.options && stage.options.record} onRecordChange={(record: boolean) => this.props.formik.setFieldValue('options.record', record)} />