Skip to content

Commit

Permalink
fix(kubernetes): Fix merge strategy field (#7455)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
ezimanyi authored Sep 27, 2019
1 parent 3b880c7 commit 479e9d6
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 35 deletions.
Original file line number Diff line number Diff line change
@@ -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<IStageConfigProps> {
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 (
<FormikStageConfig
{...this.props}
stage={this.stage}
onChange={this.props.updateStage}
render={props => <PatchManifestStageForm {...props} updatePipeline={this.props.updatePipeline} />}
render={props => (
<PatchManifestStageForm
{...props}
stageFieldUpdated={this.props.stageFieldUpdated}
updatePipeline={this.props.updatePipeline}
/>
)}
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { SelectorMode } from 'kubernetes/v2/manifest/selector/IManifestSelector'
import { PatchManifestOptionsForm } from './PatchManifestOptionsForm';

interface IPatchManifestStageConfigFormProps {
stageFieldUpdated: () => void;
updatePipeline: (pipeline: IPipeline) => void;
}

Expand Down Expand Up @@ -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<Option<string>> => {
return map([this.textSource, this.artifactSource], option => ({
Expand All @@ -114,7 +117,7 @@ export class PatchManifestStageForm extends React.Component<
<RadioButtonInput
options={this.getSourceOptions()}
onChange={(e: any) => this.props.formik.setFieldValue('source', e.target.value)}
value={stage.source || 'text'}
value={stage.source}
/>
</StageConfigField>
{stage.source === this.textSource && (
Expand Down Expand Up @@ -154,8 +157,8 @@ export class PatchManifestStageForm extends React.Component<
<hr />
<h4>Patch Options</h4>
<PatchManifestOptionsForm
strategy={!!stage.options && stage.options.strategy}
onStrategyChange={(strategy: string) => 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)}
/>
Expand Down

0 comments on commit 479e9d6

Please sign in to comment.