refactor(artifacts): consolidate artifacts feature flags checks (#8096)

* refactor(artifacts): consolidate artifacts feature flags checks * fix(artifacts): enable artifact-specific stages when only `artifactsRewrite` feature flag is enabled * fix(artifacts): make artifactsMode read-only
spinnaker · Mar 26, 2020 · 51dba01 · 51dba01
1 parent 528198a
commit 51dba01
Show file tree

Hide file tree

Showing 17 changed files with 141 additions and 92 deletions.
diff --git a/app/scripts/modules/core/src/artifact/ArtifactsModeService.ts b/app/scripts/modules/core/src/artifact/ArtifactsModeService.ts
@@ -0,0 +1,41 @@
+import { SETTINGS } from 'core/config';
+
+/**
+ * Currently, because there are two artifacts feature flags, there are four
+ * possible configuration states:
+ * 1. `artifacts` disabled, `artifactsRewrite` disabled
+ * 2. `artifacts` enabled, `artifactsRewrite` disabled
+ * 3. `artifacts` disabled, `artifactsRewrite` enabled
+ * 4. `artifacts` enabled, `artifactsRewrite` enabled
+ *
+ * However, only three UI experiences should be possible:
+ * 1. No artifacts UI
+ * 2. The legacy artifacts UI
+ * 3. The standard ("rewrite") artifacts UI
+ *
+ * This service provides a layer of abstraction over the existing feature flag
+ * checks in terms of these three possible experiences. As we deprecate these
+ * feature flags in favor of an enabled-by-default standard artifacts UI, as
+ * described in https://github.com/spinnaker/governance/pull/111, it will be
+ * helpful to have this logic isolated to a single service.
+ */
+
+export enum ArtifactsMode {
+  DISABLED,
+  LEGACY,
+  STANDARD,
+}
+
+export class ArtifactsModeService {
+  public static readonly artifactsMode = ArtifactsModeService.getArtifactsMode();
+
+  private static getArtifactsMode(): ArtifactsMode {
+    if (SETTINGS.feature.artifactsRewrite === true) {
+      return ArtifactsMode.STANDARD;
+    }
+    if (SETTINGS.feature.artifacts === true) {
+      return ArtifactsMode.LEGACY;
+    }
+    return ArtifactsMode.DISABLED;
+  }
+}
diff --git a/app/scripts/modules/core/src/artifact/expectedArtifactMultiSelector.component.ts b/app/scripts/modules/core/src/artifact/expectedArtifactMultiSelector.component.ts
@@ -1,6 +1,7 @@
 import { IComponentOptions, IController, module } from 'angular';
 import { IExpectedArtifact } from 'core/domain';
 import { ArtifactIconService } from './ArtifactIconService';
+import { ArtifactsMode, ArtifactsModeService } from './ArtifactsModeService';
 
 import './artifactSelector.less';
 
@@ -11,6 +12,7 @@ class ExpectedArtifactMultiSelectorCtrl implements IController {
   public expectedArtifacts: IExpectedArtifact[];
   public helpFieldKey: string;
   public showIcons: boolean;
+  public artifactsEnabled = ArtifactsModeService.artifactsMode !== ArtifactsMode.DISABLED;
 
   public iconPath(expected: IExpectedArtifact): string {
     const artifact = expected && (expected.matchArtifact || expected.defaultArtifact);
@@ -33,34 +35,32 @@ const expectedArtifactMultiSelectorComponent: IComponentOptions = {
   controller: ExpectedArtifactMultiSelectorCtrl,
   controllerAs: 'ctrl',
   template: `
-      <render-if-feature feature="artifacts">
-        <ng-form name="artifacts">
-          <stage-config-field label="{{ctrl.artifactLabel}}" help-key="{{ctrl.helpFieldKey}}">
-            <ui-select multiple
-                       ng-model="ctrl.command[ctrl.idsField]"
-                       class="form-control input-sm expected-artifact-multi-selector">
-              <ui-select-match>
-                <img
-                  ng-if="ctrl.showIcons && ctrl.iconPath($item)"
-                  width="16"
-                  height="16"
-                  class="artifact-icon"
-                  ng-src="{{ ctrl.iconPath($item) }}" />
-                {{ $item.displayName }}
-              </ui-select-match>
-              <ui-select-choices repeat="expected.id as expected in ctrl.expectedArtifacts">
-                <img
-                  ng-if="ctrl.showIcons && ctrl.iconPath(expected)"
-                  width="16"
-                  height="16"
-                  class="artifact-icon"
-                  ng-src="{{ ctrl.iconPath(expected) }}" />
-                <span>{{ expected.displayName }}</span>
-              </ui-select-choices>
-            </ui-select>
-          </stage-config-field>
-        </ng-form>
-      </render-if-feature>
+      <ng-form name="artifacts" ng-if="ctrl.artifactsEnabled">
+        <stage-config-field label="{{ctrl.artifactLabel}}" help-key="{{ctrl.helpFieldKey}}">
+          <ui-select multiple
+                     ng-model="ctrl.command[ctrl.idsField]"
+                     class="form-control input-sm expected-artifact-multi-selector">
+            <ui-select-match>
+              <img
+                ng-if="ctrl.showIcons && ctrl.iconPath($item)"
+                width="16"
+                height="16"
+                class="artifact-icon"
+                ng-src="{{ ctrl.iconPath($item) }}" />
+              {{ $item.displayName }}
+            </ui-select-match>
+            <ui-select-choices repeat="expected.id as expected in ctrl.expectedArtifacts">
+              <img
+                ng-if="ctrl.showIcons && ctrl.iconPath(expected)"
+                width="16"
+                height="16"
+                class="artifact-icon"
+                ng-src="{{ ctrl.iconPath(expected) }}" />
+              <span>{{ expected.displayName }}</span>
+            </ui-select-choices>
+          </ui-select>
+        </stage-config-field>
+      </ng-form>
   `,
 };
 

diff --git a/app/scripts/modules/core/src/artifact/imageSourceSelector.component.ts b/app/scripts/modules/core/src/artifact/imageSourceSelector.component.ts
@@ -1,7 +1,14 @@
 import { IComponentOptions, module } from 'angular';
 
 const imageSourceSelectorComponent: IComponentOptions = {
-  bindings: { command: '=', imageSources: '<', helpFieldKey: '@', idField: '@', imageSourceText: '<' },
+  bindings: {
+    command: '=',
+    imageSources: '<',
+    helpFieldKey: '@',
+    idField: '@',
+    imageSourceText: '<',
+    artifactsEnabled: '<',
+  },
   controllerAs: 'ctrl',
   template: `
     <div class="form-group" ng-if="ctrl.imageSourceText">
@@ -12,22 +19,20 @@ const imageSourceSelectorComponent: IComponentOptions = {
         <span ng-bind-html="ctrl.imageSourceText"></span>
       </div>
     </div>
-    <render-if-feature feature="artifacts" ng-if="!ctrl.imageSourceText">
-      <div class="form-group">
-        <div class="col-md-3 sm-label-right">
-          Image Source
-          <help-field key="{{ ctrl.helpFieldKey }}"></help-field>
-        </div>
-        <div class="col-md-7">
-          <div class="radio" ng-repeat="imageSource in ctrl.imageSources">
-            <label>
-              <input type="radio" ng-model="ctrl.command[ctrl.idField]" value="{{ imageSource }}">
-              {{ imageSource | robotToHuman }}
-            </label>
-          </div>
+    <div class="form-group" ng-if="!ctrl.imageSourceText && ctrl.artifactsEnabled">
+      <div class="col-md-3 sm-label-right">
+        Image Source
+        <help-field key="{{ ctrl.helpFieldKey }}"></help-field>
+      </div>
+      <div class="col-md-7">
+        <div class="radio" ng-repeat="imageSource in ctrl.imageSources">
+          <label>
+            <input type="radio" ng-model="ctrl.command[ctrl.idField]" value="{{ imageSource }}">
+            {{ imageSource | robotToHuman }}
+          </label>
         </div>
       </div>
-    </render-if-feature>
+    </div>
   `,
 };
 

diff --git a/app/scripts/modules/core/src/artifact/index.ts b/app/scripts/modules/core/src/artifact/index.ts
@@ -1,5 +1,6 @@
 export * from './artifactIconList';
 export * from './ArtifactIconService';
+export * from './ArtifactsModeService';
 export * from './ArtifactReferenceService';
 export * from './artifactTab';
 export * from './expectedArtifact.service';

diff --git a/app/scripts/modules/core/src/artifact/react/StageArtifactSelectorDelegate.tsx b/app/scripts/modules/core/src/artifact/react/StageArtifactSelectorDelegate.tsx
@@ -1,10 +1,10 @@
 import React from 'react';
 
 import { StageConfigField } from 'core/pipeline';
-import { SETTINGS } from 'core/config/settings';
 
-import { StageArtifactSelector, IStageArtifactSelectorProps } from './StageArtifactSelector';
-import { PreRewriteStageArtifactSelector, IPreRewriteArtifactSelectorProps } from './PreRewriteStageArtifactSelector';
+import { IStageArtifactSelectorProps, StageArtifactSelector } from './StageArtifactSelector';
+import { IPreRewriteArtifactSelectorProps, PreRewriteStageArtifactSelector } from './PreRewriteStageArtifactSelector';
+import { ArtifactsMode, ArtifactsModeService } from '../ArtifactsModeService';
 
 interface IStageArtifactSelectorDelegateProps {
   helpKey?: string;
@@ -15,7 +15,7 @@ interface IStageArtifactSelectorDelegateProps {
 export const StageArtifactSelectorDelegate = (
   props: IStageArtifactSelectorProps & IPreRewriteArtifactSelectorProps & IStageArtifactSelectorDelegateProps,
 ) => {
-  return SETTINGS.feature['artifactsRewrite'] ? (
+  return ArtifactsModeService.artifactsMode === ArtifactsMode.STANDARD ? (
     <StageConfigField label={props.label} helpKey={props.helpKey} fieldColumns={props.fieldColumns}>
       <StageArtifactSelector {...props} />
     </StageConfigField>

diff --git a/...re/src/pipeline/config/stages/findArtifactFromExecution/findArtifactFromExecutionStage.ts b/...re/src/pipeline/config/stages/findArtifactFromExecution/findArtifactFromExecutionStage.ts
@@ -1,7 +1,7 @@
 import { module } from 'angular';
 
+import { ArtifactsMode, ArtifactsModeService } from 'core/artifact';
 import { Registry } from 'core/registry';
-import { SETTINGS } from 'core/config/settings';
 
 import { ExecutionDetailsTasks } from '../common';
 import { FindArtifactFromExecutionCtrl } from '../findArtifactFromExecution/findArtifactFromExecution.controller';
@@ -12,7 +12,7 @@ export const FIND_ARTIFACT_FROM_EXECUTION_STAGE = 'spinnaker.core.pipeline.stage
 
 module(FIND_ARTIFACT_FROM_EXECUTION_STAGE, [])
   .config(() => {
-    if (SETTINGS.feature.artifacts) {
+    if (ArtifactsModeService.artifactsMode !== ArtifactsMode.DISABLED) {
       Registry.pipeline.registerStage({
         label: 'Find Artifacts From Execution',
         description: 'Find and bind artifacts from another execution',

diff --git a/app/scripts/modules/core/src/pipeline/config/stages/stage.html b/app/scripts/modules/core/src/pipeline/config/stages/stage.html
@@ -121,18 +121,19 @@ <h4 ng-bind="stage.name || '[new stage]'"></h4>
       >
       </notification-list>
     </page-section>
-    <page-section
-      key="producesArtifacts"
-      label="Produces Artifacts"
-      visible="stageProducesArtifacts() && !stageConfigCtrl.checkFeatureFlag('artifactsRewrite') && stageConfigCtrl.checkFeatureFlag('artifacts')"
-    >
-      <produces-artifacts stage="stage" pipeline="pipeline"></produces-artifacts>
+    <page-section key="producesArtifacts" label="Produces Artifacts" visible="stageProducesArtifacts()">
+      <produces-artifacts
+        ng-if="stageConfigCtrl.renderLegacyArtifactsUI"
+        stage="stage"
+        pipeline="pipeline"
+      ></produces-artifacts>
+      <produces-artifacts-react
+        ng-if="stageConfigCtrl.renderStandardArtifactsUI"
+        stage="stage"
+        pipeline="pipeline"
+        on-produces-changed="producesArtifactsChanged"
+      />
     </page-section>
-    <render-if-feature feature="artifactsRewrite">
-      <page-section key="producesArtifacts" label="Produces Artifacts" visible="stageProducesArtifacts()">
-        <produces-artifacts-react stage="stage" pipeline="pipeline" on-produces-changed="producesArtifactsChanged" />
-      </page-section>
-    </render-if-feature>
     <page-section key="comments" label="Comments" no-wrapper="true">
       <textarea
         class="form-control"

diff --git a/app/scripts/modules/core/src/pipeline/config/stages/stage.module.js b/app/scripts/modules/core/src/pipeline/config/stages/stage.module.js
@@ -7,8 +7,8 @@ import { defaultsDeep, extend, omit } from 'lodash';
 
 import { AccountService } from 'core/account/AccountService';
 import { API } from 'core/api';
+import { ArtifactsMode, ArtifactsModeService } from 'core/artifact';
 import { BASE_EXECUTION_DETAILS_CTRL } from './common/baseExecutionDetails.controller';
-import { SETTINGS } from 'core/config';
 import { ConfirmationModalService } from 'core/confirmationModal';
 import { STAGE_NAME } from './StageName';
 import { PipelineConfigService } from '../services/PipelineConfigService';
@@ -133,7 +133,8 @@ module(CORE_PIPELINE_CONFIG_STAGES_STAGE_MODULE, [
         });
       };
 
-      this.checkFeatureFlag = flag => !!SETTINGS.feature[flag];
+      this.renderLegacyArtifactsUI = ArtifactsModeService.artifactsMode === ArtifactsMode.LEGACY;
+      this.renderStandardArtifactsUI = ArtifactsModeService.artifactsMode === ArtifactsMode.STANDARD;
 
       this.editStageJson = () => {
         const modalProps = { dialogClassName: 'modal-lg modal-fullscreen' };

diff --git a/app/scripts/modules/core/src/pipeline/config/triggers/Trigger.tsx b/app/scripts/modules/core/src/pipeline/config/triggers/Trigger.tsx
@@ -5,6 +5,7 @@ import { isEqual, pick } from 'lodash';
 import { Option } from 'react-select';
 
 import { Application } from 'core/application';
+import { ArtifactsMode, ArtifactsModeService } from 'core/artifact';
 import { SETTINGS } from 'core/config/settings';
 import { IExpectedArtifact, IPipeline, ITrigger, ITriggerTypeConfig } from 'core/domain';
 import { HelpField } from 'core/help/HelpField';
@@ -122,13 +123,6 @@ function TriggerForm(triggerFormProps: ITriggerProps & { formik: FormikProps<ITr
   const expectedArtifactOptions =
     pipeline.expectedArtifacts && pipeline.expectedArtifacts.map(e => ({ label: e.displayName, value: e.id }));
 
-  const showArtifactConstraints = SETTINGS.feature['artifactsRewrite'];
-  const showOldArtifactConstraints =
-    !showArtifactConstraints &&
-    SETTINGS.feature['artifacts'] &&
-    pipeline.expectedArtifacts &&
-    pipeline.expectedArtifacts.length > 0;
-
   return (
     <fieldset disabled={trigger.inherited} className={fieldSetClassName}>
       <WatchValue
@@ -163,7 +157,7 @@ function TriggerForm(triggerFormProps: ITriggerProps & { formik: FormikProps<ITr
           />
         )}
 
-        {showOldArtifactConstraints && (
+        {ArtifactsModeService.artifactsMode === ArtifactsMode.LEGACY && pipeline.expectedArtifacts?.length > 0 && (
           <FormikFormField
             name="expectedArtifactIds"
             label="Artifact Constraints"
@@ -172,7 +166,7 @@ function TriggerForm(triggerFormProps: ITriggerProps & { formik: FormikProps<ITr
           />
         )}
 
-        {showArtifactConstraints && (
+        {ArtifactsModeService.artifactsMode === ArtifactsMode.STANDARD && (
           <FormikFormField
             name="expectedArtifactIds"
             label="Artifact Constraints"

diff --git a/app/scripts/modules/core/src/pipeline/config/triggers/Triggers.tsx b/app/scripts/modules/core/src/pipeline/config/triggers/Triggers.tsx
@@ -1,9 +1,9 @@
 import React from 'react';
 
 import { Application } from 'core/application';
+import { ArtifactsMode, ArtifactsModeService } from 'core/artifact';
 import { IPipeline } from 'core/domain';
 import { PageNavigator, PageSection } from 'core/presentation';
-import { SETTINGS } from 'core/config/settings';
 import { ExecutionOptionsPageContent } from './ExecutionOptionsPageContent';
 import { ExpectedArtifactsPageContent } from './ExpectedArtifactsPageContent';
 import { TriggersPageContent } from './TriggersPageContent';
@@ -22,10 +22,6 @@ export interface ITriggersProps {
 export function Triggers(props: ITriggersProps) {
   const { pipeline, viewState } = props;
 
-  function checkFeatureFlag(flag: string): boolean {
-    return !!SETTINGS.feature[flag];
-  }
-
   // KLUDGE: This value is used as a React key when rendering the Triggers.
   // Whenever the pipeline is reverted, this causes the Triggers to remount and reset formik state.
   const revertCountKLUDGE = viewState.revertCount;
@@ -39,7 +35,7 @@ export function Triggers(props: ITriggersProps) {
         label="Expected Artifacts"
         badge={pipeline.expectedArtifacts ? pipeline.expectedArtifacts.length.toString() : '0'}
         noWrapper={true}
-        visible={!checkFeatureFlag('artifactsRewrite') && checkFeatureFlag('artifacts')}
+        visible={ArtifactsModeService.artifactsMode === ArtifactsMode.LEGACY}
       >
         <ExpectedArtifactsPageContent {...props} />
       </PageSection>

diff --git a/app/scripts/modules/core/src/pipeline/config/triggers/TriggersPageContent.tsx b/app/scripts/modules/core/src/pipeline/config/triggers/TriggersPageContent.tsx
@@ -3,7 +3,7 @@ import React from 'react';
 import { findIndex } from 'lodash';
 
 import { Application } from 'core/application';
-import { ArtifactReferenceService } from 'core/artifact/ArtifactReferenceService';
+import { ArtifactReferenceService, ArtifactsMode, ArtifactsModeService } from 'core/artifact';
 import { IExpectedArtifact, IPipeline, ITrigger } from 'core/domain';
 import { HelpField } from 'core/help';
 import { PipelineConfigValidator } from '../validation/PipelineConfigValidator';
@@ -65,7 +65,7 @@ export function TriggersPageContent(props: ITriggersPageContentProps) {
     updatedTriggers[index] = updatedTrigger;
     PipelineConfigValidator.validatePipeline(pipeline);
     updatePipelineConfig({ triggers: updatedTriggers });
-    if (SETTINGS.feature['artifactsRewrite']) {
+    if (ArtifactsModeService.artifactsMode === ArtifactsMode.STANDARD) {
       removeUnusedExpectedArtifacts(pipeline);
     }
   }

diff --git a/app/scripts/modules/core/src/pipeline/status/ParametersAndArtifacts.tsx b/app/scripts/modules/core/src/pipeline/status/ParametersAndArtifacts.tsx
@@ -2,8 +2,8 @@ import React from 'react';
 import { keyBy, truncate } from 'lodash';
 import memoizeOne from 'memoize-one';
 
+import { ArtifactsMode, ArtifactsModeService } from 'core/artifact';
 import { IExecution, IPipeline } from 'core/domain';
-import { SETTINGS } from 'core/config/settings';
 
 import { ExecutionParameters, IDisplayableParameter } from './ExecutionParameters';
 import { ResolvedArtifactList } from './ResolvedArtifactList';
@@ -132,7 +132,7 @@ export class ParametersAndArtifacts extends React.Component<
           pinnedDisplayableParameters={pinnedDisplayableParameters}
         />
 
-        {(SETTINGS.feature.artifacts || SETTINGS.feature.artifactsRewrite) && (
+        {ArtifactsModeService.artifactsMode !== ArtifactsMode.DISABLED && (
           <ResolvedArtifactList
             artifacts={artifacts}
             resolvedExpectedArtifacts={resolvedExpectedArtifacts}

diff --git a/...ipts/modules/google/src/serverGroup/configure/wizard/location/basicSettings.controller.js b/...ipts/modules/google/src/serverGroup/configure/wizard/location/basicSettings.controller.js
@@ -5,6 +5,8 @@ import { Observable, Subject } from 'rxjs';
 import { extend } from 'lodash';
 
 import {
+  ArtifactsMode,
+  ArtifactsModeService,
   ArtifactTypePatterns,
   ExpectedArtifactSelectorViewController,
   excludeAllTypesExcept,
@@ -141,5 +143,7 @@ angular
         delegate: gceImageDelegate,
         controller: new ExpectedArtifactSelectorViewController(gceImageDelegate),
       };
+
+      this.artifactsEnabled = ArtifactsModeService.artifactsMode !== ArtifactsMode.DISABLED;
     },
   ]);