-
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(kubernetes): add expression evaluation options to bake and deplo… #6696
feat(kubernetes): add expression evaluation options to bake and deplo… #6696
Conversation
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.
🔥 👍
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.
Code lgtm. Only suggestion I would have is to add info tooltips to the checkbox labels that explain the reason that a user would want to enable / disable evaluation.
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.
Looks good! One question
@@ -41,6 +41,14 @@ <h4>Manifest Configuration</h4> | |||
> | |||
</stage-artifact-selector-react> | |||
</stage-config-field> | |||
<stage-config-field label="Expression Evaluation" ng-if="ctrl.$scope.stage.source === ctrl.artifactSource"> |
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.
Do we only want to show the textbox if the manifest comes from an artifact? I guess that's probably the most common use case, as if it's a text box, you can probably just not type SpEL in the first place...but just wondering if the checkbox should be there for both.
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.
That was my initial thought as well, but it seems misleading since as far as I can tell we are only explicitly evaluating (and failing the stage on the failure to evaluate) SpEL expressions for manifest artifacts in the Deploy Stage.
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.
Oh, good catch---I had no idea SpEL evaluation only happened for artifacts. Makes sense to only show the checkbox in that case then.
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.
Confusingly, I think it happens for text manifests as well, during one of the possibly many non-failing passes at expression evaluation that occur when the pipeline begins. We're explicitly evaluating SpEL in artifact manifests just prior to deploy since the full manifest text may only be available after those initial passes.
@sbwsg Great idea on tooltip text, how does this sound? Bake stage: Deploy stage: |
…y manifest stages
d0b794d
to
a5a099c
Compare
Nice, lgtm! |
#6726) * chore(core): Bump version to 0.0.345 29b001b fix({core,cloudfoundry}/deploy): better red/black, rolling red/black help text (#6699) 647e331 perf(core): cache subnets in window session (#6717) 5b6f229 refactor(core): use non-deprecated tasks endpoint for app tasks (#6721) 061a60c fix(triggers): Add timeout to polling on manual trigger (#6707) 40c5e09 fix(concourse): Fix concourse trigger config (#6715) 2ac15b2 fix(artifacts): Fix SpEL text input used in React components (#6712) 8c4104f fix(trigger): Fix react trigger components don't receive prop updates (#6711) a5a54bd feat(kubernetes): add expression evaluation options to bake and deploy manifest stages (#6696) 78786f3 fix(artifacts): Fix trigger artifact feature conditionalization (#6708) 04746e0 fix(artifacts): HTTP artifact needs to set the reference field (#6679) 51eeba4 chore(core): upgrade the version to formik 1.4.1 (#6705) 4f826d1 fix(cf): fix the alignment issue for artifacts in deploy SG (#6701) 2d9fb15 fix(triggers): Add pipeline name to search request (#6703) 75de845 feat(triggers): Add Concourse trigger type (#6692) e2940b4 feat(cf): Share/Unshare services (#6685) 88c8b5d feat(core): save pipelines stage was added (#6654) 4a95a78 fix(artifacts): Make artifacts and artifactsRewrite flags mutually exclusive (#6694) 50cffc9 chore(artifactory): Polish imports and labels (#6693) 4ac54ce feat(core/deploy): UI widget for 'Delay Before Scale Down' (#6698) 792fcaa feat(core): add jenkins artifact type (#6690) 7f68f22 refactor(core): Remove triggerViaEcho feature flag (#6680) * chore(amazon): Bump version to 0.0.181 51eeba4 chore(core): upgrade the version to formik 1.4.1 (#6705) 515bccb feature(aws): Add the cloudformation stage (#6521) * chore(titus): Bump version to 0.0.80 33d674d fix(titus): use arrow function to avoid bad this binding in callback (#6724) 8a6cdfc feat(titus): Surface env and resources for runjob stage (#6697) 51eeba4 chore(core): upgrade the version to formik 1.4.1 (#6705)
1ada021 feat(kubernetes): expose rendered helm template in execution details (spinnaker#6943) d122749 feat(runJob/kubernetes): render external link (spinnaker#6930) 9e03e52 refactor(runJob/kubernetes): refactor exec details (spinnaker#6924) 98ca0ff refactor(runJob/kubernetes): use joblogviewer (spinnaker#6917) 6c5eed6 feat(provider/kubernetes): run job manifest artifacts (spinnaker#6902) cd8780c fix(kubernetes): fix discrepancy between orca deploy stage model and artifacts rewrite stage model d44be0c fix(kubernetes): hide manifest artifact selector in text mode bd94593 feat(kubernetes): remove rollout strategies feature flag 0ec6a7e feat(kubernetes): validate text manifests when rollout strategies enabled 37e24d7 fix(kubernetes): safer lookups for deploy stage validators (spinnaker#6847) 478e326 feat(kubernetes): add rollout strategies to deploy manifest stage (spinnaker#6841) e618548 feat(preconfiguredJob): logs for k8s jobs (spinnaker#6840) c30058a feat(k8s): Exclude inline base 64 artifact editing in k8s manifest (spinnaker#6839) 5640431 fix(k8s): Fix deploy manifest (spinnaker#6833) ee89a64 feat(kuberntes): v2 runJob (spinnaker#6831) 2d7f388 feat(kubernetes): feature-flagged support for kubernetes traffic management strategies (spinnaker#6816) 33ad914 chore(kubernetes): refactor BasicSettings component to be usable in stages (spinnaker#6820) 72e164d refactor(core): de-angularize ApplicationModelBuilder, fix project executions (spinnaker#6802) d7f6fbd fix(kubernetes): show Deployment clusters in Find Artifacts from Resource stages (spinnaker#6794) 20d0d7a fix(kubernetes): safe lookups for apiVersion on patch manifest deploy status (spinnaker#6775) bf13a3c fix(kubernetes): fix validation for Find Artifact from Resource stage (spinnaker#6777) 50b74f2 feat(gcb): add Google Cloud Build stage (spinnaker#6774) 5b03078 fix(kubernetes): fix copy manifest from infrastructure button (spinnaker#6719) 57c30b5 fix(kubernetes): Use apiGroup when looking up deploy status for CRDs (spinnaker#6691) a5a54bd feat(kubernetes): add expression evaluation options to bake and deploy manifest stages (spinnaker#6696) 51eeba4 chore(core): upgrade the version to formik 1.4.1 (spinnaker#6705) e2b4d8e refactor(*): remove unused local storage caches (spinnaker#6665) 5da2965 refactor(artifacts): Combine expected artifacts and trigger artifact constraints (spinnaker#6634) 0d37cb2 fix(kubernetes): only set manifestName in static mode manifest selector 79a0700 feat(kubernetes): add dynamic target selection to patch manifest stage 0abce66 fix(kubernetes): remove unnecessary delete manifest stage defaults 5bf6045 fix(kubernetes): allow text input for replicas in `Scale (Manifest)` stage (spinnaker#6630) cbfcae4 feat(kubernetes): add label mode to manifest selector component to enable dynamic target selection in delete manifest stage (spinnaker#6628) d828a53 chore(angularjs): Explicitly annotate directive controllers 3e75815 refactor(core): migrate momentjs functionality to luxon + date-fns (spinnaker#6604) 7d5fc34 chore(prettier): Just Use Prettier™ (spinnaker#6600) 04bb4a0 fix(html): Fix various invalid HTML (spinnaker#6599) 5cf6c79 chore(prettier): Just Use Prettier™ 3ffa4fb chore(angularjs): Do not use .component('foo', new Foo()) 4b91b36 refactor(core): move Ace Editor CSS to core module (spinnaker#6588) cc52bee chore(angularjs): Remove all 'ngInject'; in favor of explicit DI annotation b6bab1e chore(prettier): Just Use Prettier™ f3fd790 chore(angularjs): Explicitly annotate all AngularJS injection points 629613f fix(kubernetes): properly detect if autoscaler is attached to server groups (spinnaker#6578) d72bc17 fix(eslint): Fix eslint warnings for @typescript-eslint/camelcase
1ada021 feat(kubernetes): expose rendered helm template in execution details (#6943) d122749 feat(runJob/kubernetes): render external link (#6930) 9e03e52 refactor(runJob/kubernetes): refactor exec details (#6924) 98ca0ff refactor(runJob/kubernetes): use joblogviewer (#6917) 6c5eed6 feat(provider/kubernetes): run job manifest artifacts (#6902) cd8780c fix(kubernetes): fix discrepancy between orca deploy stage model and artifacts rewrite stage model d44be0c fix(kubernetes): hide manifest artifact selector in text mode bd94593 feat(kubernetes): remove rollout strategies feature flag 0ec6a7e feat(kubernetes): validate text manifests when rollout strategies enabled 37e24d7 fix(kubernetes): safer lookups for deploy stage validators (#6847) 478e326 feat(kubernetes): add rollout strategies to deploy manifest stage (#6841) e618548 feat(preconfiguredJob): logs for k8s jobs (#6840) c30058a feat(k8s): Exclude inline base 64 artifact editing in k8s manifest (#6839) 5640431 fix(k8s): Fix deploy manifest (#6833) ee89a64 feat(kuberntes): v2 runJob (#6831) 2d7f388 feat(kubernetes): feature-flagged support for kubernetes traffic management strategies (#6816) 33ad914 chore(kubernetes): refactor BasicSettings component to be usable in stages (#6820) 72e164d refactor(core): de-angularize ApplicationModelBuilder, fix project executions (#6802) d7f6fbd fix(kubernetes): show Deployment clusters in Find Artifacts from Resource stages (#6794) 20d0d7a fix(kubernetes): safe lookups for apiVersion on patch manifest deploy status (#6775) bf13a3c fix(kubernetes): fix validation for Find Artifact from Resource stage (#6777) 50b74f2 feat(gcb): add Google Cloud Build stage (#6774) 5b03078 fix(kubernetes): fix copy manifest from infrastructure button (#6719) 57c30b5 fix(kubernetes): Use apiGroup when looking up deploy status for CRDs (#6691) a5a54bd feat(kubernetes): add expression evaluation options to bake and deploy manifest stages (#6696) 51eeba4 chore(core): upgrade the version to formik 1.4.1 (#6705) e2b4d8e refactor(*): remove unused local storage caches (#6665) 5da2965 refactor(artifacts): Combine expected artifacts and trigger artifact constraints (#6634) 0d37cb2 fix(kubernetes): only set manifestName in static mode manifest selector 79a0700 feat(kubernetes): add dynamic target selection to patch manifest stage 0abce66 fix(kubernetes): remove unnecessary delete manifest stage defaults 5bf6045 fix(kubernetes): allow text input for replicas in `Scale (Manifest)` stage (#6630) cbfcae4 feat(kubernetes): add label mode to manifest selector component to enable dynamic target selection in delete manifest stage (#6628) d828a53 chore(angularjs): Explicitly annotate directive controllers 3e75815 refactor(core): migrate momentjs functionality to luxon + date-fns (#6604) 7d5fc34 chore(prettier): Just Use Prettier™ (#6600) 04bb4a0 fix(html): Fix various invalid HTML (#6599) 5cf6c79 chore(prettier): Just Use Prettier™ 3ffa4fb chore(angularjs): Do not use .component('foo', new Foo()) 4b91b36 refactor(core): move Ace Editor CSS to core module (#6588) cc52bee chore(angularjs): Remove all 'ngInject'; in favor of explicit DI annotation b6bab1e chore(prettier): Just Use Prettier™ f3fd790 chore(angularjs): Explicitly annotate all AngularJS injection points 629613f fix(kubernetes): properly detect if autoscaler is attached to server groups (#6578) d72bc17 fix(eslint): Fix eslint warnings for @typescript-eslint/camelcase
…y manifest stages
Corresponding Orca PR: spinnaker/orca#2761
The primary purpose of this change is to prevent Deploy Manifest stage failure for users attempting to deploy third-party Helm manifests that include
${}
-wrapped expressions, while allowing SpEL overrides to be explicitly evaluated just prior to baking. These options should probably be deprecated when expression evaluation is refactored in Spinnaker (see this proposal).evaluateOverrideExpressions
option to Bake Manifest stage. This defaults to false. When it is set to true, SpEL expressions evaluation of manifest overrides will occur during the Bake stage.skipExpressionEvaluation
option to Deploy Manifest stage for manifest artifacts. This defaults to false. When it is set to true, SpEL expression of the manifest artifact will not occur during the Deploy stage.${}
-wrapped expressions not intended for Spinnaker.Related issues:
Before:
After: