-
Notifications
You must be signed in to change notification settings - Fork 174
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
Variable all the things! #613
Comments
@Racer159 added some more details to the issue if you want to pick this one up |
@jeff-mccoy @RothAndrew @YrrepNoj is this worthy of and ADR as to why we chose to go with variables at the package level (vs component level) and why we changed from simple key-value pair map to a list of sub-objects? |
I think the quick and easy answer is if it was worth asking that question then the answer likely should be yes, but happy to hear other opinions |
I don't think it warrants an adr as we haven't done one for any other package structure changes (of which there have been dozens) since we aren't GA. It certainly should be explained in the PR (or issue) why we changed the variable placement. We currently have ADRs for large architecture decisions such as the injector system, major test replacement and the mutating webhook. This doesn't seem like that level of change to me. |
@jeff-mccoy @RothAndrew @YrrepNoj how did you want to handle variables in nested packages/components? I see two potential paths:
|
Path 2 has been chosen |
Yeah it's funny looking, but I don't think we'd want to change that. I would note though, the package should not include the REGISTRY variables as that is specified in the manifest. If it is specified in the |
Yeah it makes sense since you don't know what that might be overridden with later (and it is nice to know that it is dynamic in the output). Was mostly just poking around/asking because there are no tests covering it right now. And yes those builtins I left prefixed with |
Yeah I don't think we have hardly any test coverage for |
Updated the issue with all the tweaks we discussed during our meeting earlier @Racer159 @YrrepNoj @Madeline-UX |
@jeff-mccoy @Madeline-UX @YrrepNoj close to having this ready I think, but thoughts on differentiating the create variables even further with something like |
Yeah I'm certainly happy with being more explicit with this. Not sure which format is better, no strongly held opinion as I still see it as an overall edge case. |
Going with |
@Racer159 @YrrepNoj and myself got together to document the current pain points, desired outcomes, Solutions, and updated user flows for this enhancement. This design references functional users (user defined by action they are trying to complete) Not to be confused with personas. There may be multiple personas that fit each functional role. For reference to the living document see Miro >> https://miro.com/app/board/uXjVOUJ4f2s=/?moveToWidget=3458764529916551885&cot=14 *living documents may be updated in the future to reflect new information. Here is a screen capture of that miro board at the time of this PR being reviewed. |
Move component-level variables to the package level and allow package string-templating for the Zarf.yaml and package variables templated out manifests.
Create-time variables
Zarf.yaml string templating
package create
variables
orconstants
listszarf.yaml
package create
Deploy-time constants
Constant declaration by package authors
constants
keypackage create
package deploy
ZARF_CONST
prefixIn the manifest will appear as
###ZARF_CONST_AGENT_IMAGE###
Deploy-time variables
Variable declaration by package deployers
variables
keydefault
key--set
flags onpackage deploy
package deploy
ifprompt: true
ZARF_VAR_
prefixIn the manifest will appear as
###ZARF_VAR_AGENT_IMAGE###
and###ZARF_VAR_PROMPT_ON_DEPLOY###
Issue planning coordinated with @RothAndrew @YrrepNoj @Racer159
The text was updated successfully, but these errors were encountered: