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

Declared variables not reflected in package preview prompt #1124

Closed
blancharda opened this issue Dec 14, 2022 · 7 comments · Fixed by #1682
Closed

Declared variables not reflected in package preview prompt #1124

blancharda opened this issue Dec 14, 2022 · 7 comments · Fixed by #1682
Assignees
Labels
bug 🐞 Something isn't working
Milestone

Comments

@blancharda
Copy link

Overview

When overriding a variable using --set at deploy time, the default value (rather than the declared value) is shown in the package preview. The variable is correctly overriden if the user continues with the deploy, and the prompt is technically correct -- it explicitly shows the default value - but the behavior seems unclear / unexpected.

Environment

OS: MacOS Ventura, Ubuntu
App version: 0.22.2

Steps to reproduce

  1. Declare a variable in a package
  2. Override that variable's default using zarf package deploy --set <var>=<value>
  3. Observe the displayed package information at the prompt

Expected result

Variable should indicate the <value> provided via --set

Actual Result

Variable indicates the default value defined int the zarf yaml

Example

zarf.yaml:

kind: ZarfPackageConfig
metadata:
  name: var-example
  architecture: amd64

variables:
  - name: FOO
    default: "BAR"
    prompt: false

Sample deploy prompt:

$ zarf package deploy zarf-package-example.tar.zst --set FOO=BAZ

Saving log file to
/var/folders/xx/xh0zby_504s20_smr55cz0g00000gn/T/zarf-2022-12-14-13-22-43-514321233.log
  ✔  Preparing zarf package zarf-package-example.tar.zst

kind: ZarfPackageConfig
metadata:
  name: var-example
  description: ""
  version: ""
  url: ""
  image: ""
  uncompressed: false
  architecture: amd64
build:
  terminal: fruit-basket.local
  user: ablanchard
  architecture: amd64
  timestamp: Wed, 14 Dec 2022 13:22:20 -0500
  version: v0.22.2
components: []
variables:
- name: FOO
  description: ""
  default: BAR
  prompt: false
constants: []

? Deploy this Zarf package? (y/N)
...
@blancharda
Copy link
Author

I think my ideal output would indicate a diff from the defaults -- something like:

default: BAR --> BAZ

But I don't see a good way to get there without hacking apart the yaml printing or (jankily) replacing the in-memory default string.

It's not pretty, and I'm not sure I'm advocating for it... but for discussion's sake..
If we were to move this initialization block for SetVariables into the packager constructor, we could also update the corresponding cfg.Pkg.Variables[?].Default to be something like BAR -> BAZ. Theoretically that should be safe, since we already know the true default value is being replaced according to a flag, and then wherever it was printed, we would see the override as well as the default value for context. That feels like an abuse of the default field though..

@jeff-mccoy jeff-mccoy added this to the v0.25.x milestone Jan 17, 2023
@Racer159 Racer159 modified the milestones: v0.25.x, v0.25.y Feb 26, 2023
@Racer159 Racer159 modified the milestones: v0.25.x, v0.26-rc Mar 19, 2023
@dgershman
Copy link
Contributor

Here is some inspiration which is how Terraform shows diffs on a plan.

MnMYr (1)

@blancharda
Copy link
Author

Having the ability to see what a zarf package deploy is going to change in the cluster (perhaps in a similar style to terraform apply as suggested above) would be a huge benefit. Especially during updates and "day 2" deploys.

Should we spin that off into a separate issue?

@Racer159 Racer159 added bug 🐞 Something isn't working and removed possible-bug 🐛 labels Apr 18, 2023
@Racer159 Racer159 modified the milestones: v0.26.0, v0.26.2 Apr 18, 2023
@Racer159
Copy link
Contributor

Racer159 commented May 2, 2023

Kicking back to 5/9 milestone.

@Racer159 Racer159 self-assigned this May 4, 2023
@Racer159
Copy link
Contributor

Racer159 commented May 5, 2023

Also agree @blancharda - we probably wouldn't be able to do it (at least well) on the k8s resources themselves but could diff the package yaml in the package we are about to deploy with the package yaml that is in the package secret (if it exists) in the cluster.

@Racer159
Copy link
Contributor

Racer159 commented May 8, 2023

Kicking back to 5/16 to get more design cycles in

@Racer159
Copy link
Contributor

Racer159 commented May 18, 2023

After design cycles from @Madeline-UX the TODO is:

  • Make top-level YAML keys bg bright white fg black when printed

  • Use fg dim white to add help text for each top-level YAML key

    • i.e. Variables are configurable values that can be set for a package deployment
  • Use fg dim white to add set information to each variable -name line

    • i.e. This variable is set to 'Value value value' from 'zarf-config.toml'
    • The value should be shortened with ellipsis to fit on one line
    • The value should be sanitized if it is a sensitive value

    Design Files

    Figma File - https://www.figma.com/file/MUItIzpzLBLuIyt225Bwgl/Zarf?type=design&node-id=1551%3A57164&t=6r9vpYOF5HhaYeug-1

SBOM Color Change

Racer159 added a commit that referenced this issue Jun 22, 2023
## Description

This adds set variables to the package preview prompt

## Related Issue

Fixes #1124 

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [ ] Test, docs, adr added or updated as needed
- [X] [Contributor Guide
Steps](https://github.com/defenseunicorns/zarf/blob/main/CONTRIBUTING.md#developer-workflow)
followed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants