feat: lint unused variables in Zarf packages

adr/rules-for-var-lint.md

@@ -0,0 +1,14 @@
+The following are the rules we follow for zarf lint on files
+
+- A variable can be declared by a user in three ways. Using --set, zarf config, or zarf variables
+- Variables and constants work the same for these rules
+- Builtin variables are not looked at
+- A variable can be used in three ways. In files, a helm chart or manifests
+  - If a variable is declared by a user and not used anywhere in the package then the user should get an error in lint:  unused variable
+    - If a variable is declared by an imported component we will skip it if it is not used by the components we import as we don't know if it's actually being used or not.
+  - If a variable is used by a package and not declared anywhere by a user then the user should get an error in lint: variable not declared
+    - If a variable is declared by the imported package, the user should not recieve an error
+
+
+- Are there any files in the helm chart I should ignore? Or should I simply check of all them?
+- It feels like I should be using addComponent or at least a lot of the logic in it since it already

src/cmd/dev.go

@@ -234,9 +234,12 @@ var devLintCmd = &cobra.Command{
 	Run: func(cmd *cobra.Command, args []string) {
 		common.SetBaseDirectory(args, &pkgConfig)
 		v := common.GetViper()
+
 		pkgConfig.CreateOpts.SetVariables = helpers.TransformAndMergeMap(
 			v.GetStringMapString(common.VPkgCreateSet), pkgConfig.CreateOpts.SetVariables, strings.ToUpper)
-		validator, err := lint.Validate(pkgConfig.CreateOpts)
+		pkgConfig.PkgOpts.SetVariables = helpers.TransformAndMergeMap(
+			v.GetStringMapString(common.VPkgDeploySet), pkgConfig.PkgOpts.SetVariables, strings.ToUpper)
+		validator, err := lint.Validate(&pkgConfig)
 		if err != nil {
 			message.Fatal(err, err.Error())
 		}
@@ -268,8 +271,10 @@ func init() {
 	// allow for the override of the default helm KubeVersion
 	devFindImagesCmd.Flags().StringVar(&pkgConfig.FindImagesOpts.KubeVersionOverride, "kube-version", "", lang.CmdDevFlagKubeVersion)
 
-	devLintCmd.Flags().StringToStringVar(&pkgConfig.CreateOpts.SetVariables, "set", v.GetStringMapString(common.VPkgCreateSet), lang.CmdPackageCreateFlagSet)
+	devLintCmd.Flags().StringToStringVar(&pkgConfig.CreateOpts.SetVariables, "create-set", v.GetStringMapString(common.VPkgCreateSet), lang.CmdPackageCreateFlagSet)
+	devLintCmd.Flags().StringToStringVar(&pkgConfig.PkgOpts.SetVariables, "deploy-set", v.GetStringMapString(common.VPkgDeploySet), lang.CmdPackageDeployFlagSet)
 	devLintCmd.Flags().StringVarP(&pkgConfig.CreateOpts.Flavor, "flavor", "f", v.GetString(common.VPkgCreateFlavor), lang.CmdPackageCreateFlagFlavor)
+
 	devTransformGitLinksCmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.PushUsername, "git-account", config.ZarfGitPushUser, lang.CmdDevFlagGitAccount)
 }
 

src/internal/packager/helm/post-render.go

@@ -53,6 +53,7 @@ func (h *Helm) newRenderer() (*renderer, error) {
 	}, nil
 }
 
+// My goal at the moment is to be in the state of this function
 func (r *renderer) Run(renderedManifests *bytes.Buffer) (*bytes.Buffer, error) {
 	// This is very low cost and consistent for how we replace elsewhere, also good for debugging
 	tempDir, err := utils.MakeTempDir(r.chartPath)

src/internal/packager/template/template.go

@@ -17,6 +17,11 @@ import (
 	"github.com/defenseunicorns/zarf/src/pkg/utils/helpers"
 )
 
+const (
+	depMarkerOld = "DATA_INJECTON_MARKER"
+	depMarkerNew = "DATA_INJECTION_MARKER"
+)
+
 // Values contains the values to be used in the template.
 type Values struct {
 	config   *types.PackagerConfig
@@ -75,12 +80,6 @@ func (values *Values) GetRegistry() string {
 func (values *Values) GetVariables(component types.ZarfComponent) (templateMap map[string]*utils.TextTemplate, deprecations map[string]string) {
 	templateMap = make(map[string]*utils.TextTemplate)
 
-	depMarkerOld := "DATA_INJECTON_MARKER"
-	depMarkerNew := "DATA_INJECTION_MARKER"
-	deprecations = map[string]string{
-		fmt.Sprintf("###ZARF_%s###", depMarkerOld): fmt.Sprintf("###ZARF_%s###", depMarkerNew),
-	}
-
 	if values.config.State != nil {
 		regInfo := values.config.State.RegistryInfo
 		gitInfo := values.config.State.GitServer
@@ -159,6 +158,7 @@ func (values *Values) GetVariables(component types.ZarfComponent) (templateMap m
 		}
 	}
 
+	deprecations = GetTemplateDeprecations()
 	debugPrintTemplateMap(templateMap)
 	message.Debugf("deprecations = %#v", deprecations)
 
@@ -178,6 +178,14 @@ func (values *Values) Apply(component types.ZarfComponent, path string, ignoreRe
 	return err
 }
 
+// GetTemplateDeprecations returns a map of deprecated zarf template variables
+func GetTemplateDeprecations() map[string]string {
+	deprecations := map[string]string{
+		fmt.Sprintf("###ZARF_%s###", depMarkerOld): fmt.Sprintf("###ZARF_%s###", depMarkerNew),
+	}
+	return deprecations
+}
+
 func debugPrintTemplateMap(templateMap map[string]*utils.TextTemplate) {
 	debugText := "templateMap = { "
 

src/pkg/packager/actions.go

@@ -111,8 +111,8 @@ retryCmd:
 
 			// If an output variable is defined, set it.
 			for _, v := range action.SetVariables {
-				p.setVariableInConfig(v.Name, out, v.Sensitive, v.AutoIndent, v.Type)
-				if err := p.checkVariablePattern(v.Name, v.Pattern); err != nil {
+				p.cfg.SetVariableMap.SetVariableInConfig(v.Name, out, v.Sensitive, v.AutoIndent, v.Type)
+				if err := p.cfg.SetVariableMap.CheckVariablePattern(v.Name, v.Pattern); err != nil {
 					message.WarnErr(err, err.Error())
 					return err
 				}
@@ -206,6 +206,8 @@ func convertWaitToCmd(wait types.ZarfComponentActionWait, timeout *int) (string,
 }
 
 // Perform some basic string mutations to make commands more useful.
+// I will also have to edit to include / edit this to make sure we check for variables here as well
+// Maybe? Or maybe this is something else entirely
 func actionCmdMutation(cmd string, shellPref types.ZarfComponentActionShell) (string, error) {
 	zarfCommand, err := utils.GetFinalExecutableCommand()
 	if err != nil {

src/pkg/packager/common.go

@@ -94,7 +94,7 @@ func New(cfg *types.PackagerConfig, mods ...Modifier) (*Packager, error) {
 	}
 
 	if cfg.SetVariableMap == nil {
-		cfg.SetVariableMap = make(map[string]*types.ZarfSetVariable)
+		cfg.SetVariableMap = make(types.ZarfSetVariableMap)
 	}
 
 	var (

src/pkg/packager/compose.go

@@ -47,6 +47,7 @@ func (p *Packager) composeComponents() error {
 		components = append(components, *composed)
 
 		// merge variables and constants
+		// TODO aabro this is something I need to use
 		pkgVars = chain.MergeVariables(pkgVars)
 		pkgConsts = chain.MergeConstants(pkgConsts)
 	}

src/pkg/packager/composer/list.go

@@ -290,7 +290,7 @@ func (ic *ImportChain) Compose() (composed *types.ZarfComponent, err error) {
 	// start overriding with the tail node
 	node := ic.tail
 	for node != nil {
-		fixPaths(&node.ZarfComponent, node.relativeToHead)
+		FixPaths(&node.ZarfComponent, node.relativeToHead)
 
 		// perform overrides here
 		err := overrideMetadata(composed, node.ZarfComponent)

src/pkg/packager/composer/pathfixer.go

@@ -20,7 +20,7 @@
 	return filepath.Join(relativeTo, path)
 }
 
-func fixPaths(child *types.ZarfComponent, relativeToHead string) {
+func FixPaths(child *types.ZarfComponent, relativeToHead string) {
 	for fileIdx, file := range child.Files {
 		composed := makePathRelativeTo(file.Source, relativeToHead)
 		child.Files[fileIdx].Source = composed

src/pkg/packager/deploy.go

@@ -23,6 +23,7 @@ import (
 	"github.com/defenseunicorns/zarf/src/pkg/k8s"
 	"github.com/defenseunicorns/zarf/src/pkg/layout"
 	"github.com/defenseunicorns/zarf/src/pkg/message"
+	"github.com/defenseunicorns/zarf/src/pkg/packager/variable"
 	"github.com/defenseunicorns/zarf/src/pkg/transform"
 	"github.com/defenseunicorns/zarf/src/pkg/utils"
 	"github.com/defenseunicorns/zarf/src/pkg/utils/helpers"
@@ -62,7 +63,7 @@ func (p *Packager) Deploy() (err error) {
 	}
 
 	// Set variables and prompt if --confirm is not set
-	if err := p.setVariableMapInConfig(); err != nil {
+	if err := variable.SetVariableMapInConfig(*p.cfg); err != nil {
 		return fmt.Errorf("unable to set the active variables: %w", err)
 	}
 

src/pkg/packager/dev.go

@@ -11,6 +11,7 @@ import (
 	"github.com/defenseunicorns/zarf/src/config"
 	"github.com/defenseunicorns/zarf/src/internal/packager/validate"
 	"github.com/defenseunicorns/zarf/src/pkg/message"
+	"github.com/defenseunicorns/zarf/src/pkg/packager/variable"
 	"github.com/defenseunicorns/zarf/src/types"
 )
 
@@ -61,7 +62,7 @@ func (p *Packager) DevDeploy() error {
 	message.HeaderInfof("📦 PACKAGE DEPLOY %s", p.cfg.Pkg.Metadata.Name)
 
 	// Set variables and prompt if --confirm is not set
-	if err := p.setVariableMapInConfig(); err != nil {
+	if err := variable.SetVariableMapInConfig(*p.cfg); err != nil {
 		return fmt.Errorf("unable to set the active variables: %w", err)
 	}
 

src/pkg/packager/lint/lint.go

@@ -15,8 +15,8 @@ import (
 	"github.com/defenseunicorns/zarf/src/config"
 	"github.com/defenseunicorns/zarf/src/config/lang"
 	"github.com/defenseunicorns/zarf/src/pkg/layout"
-	"github.com/defenseunicorns/zarf/src/pkg/packager"
 	"github.com/defenseunicorns/zarf/src/pkg/packager/composer"
+	"github.com/defenseunicorns/zarf/src/pkg/packager/variable"
 	"github.com/defenseunicorns/zarf/src/pkg/transform"
 	"github.com/defenseunicorns/zarf/src/pkg/utils"
 	"github.com/defenseunicorns/zarf/src/pkg/utils/helpers"
@@ -33,25 +33,49 @@ func getSchemaFile() ([]byte, error) {
 
 // Validate validates a zarf file against the zarf schema, returns *validator with warnings or errors if they exist
 // along with an error if the validation itself failed
-func Validate(createOpts types.ZarfCreateOptions) (*Validator, error) {
+func Validate(cfg *types.PackagerConfig) (*Validator, error) {
 	validator := Validator{}
 	var err error
 
-	if err := utils.ReadYaml(filepath.Join(createOpts.BaseDir, layout.ZarfYAML), &validator.typedZarfPackage); err != nil {
+	if err := utils.ReadYaml(filepath.Join(cfg.CreateOpts.BaseDir, layout.ZarfYAML), &validator.typedZarfPackage); err != nil {
 		return nil, err
 	}
 
-	if err := utils.ReadYaml(filepath.Join(createOpts.BaseDir, layout.ZarfYAML), &validator.untypedZarfPackage); err != nil {
+	if err := utils.ReadYaml(filepath.Join(cfg.CreateOpts.BaseDir, layout.ZarfYAML), &validator.untypedZarfPackage); err != nil {
 		return nil, err
 	}
 
-	if err := os.Chdir(createOpts.BaseDir); err != nil {
-		return nil, fmt.Errorf("unable to access directory '%s': %w", createOpts.BaseDir, err)
+	if err := os.Chdir(cfg.CreateOpts.BaseDir); err != nil {
+		return nil, fmt.Errorf("unable to access directory '%s': %w", cfg.CreateOpts.BaseDir, err)
 	}
 
-	validator.baseDir = createOpts.BaseDir
+	if err := variable.SetVariableMapInConfig(*cfg); err != nil {
+		return nil, fmt.Errorf("unable to set the active variables: %w", err)
+	}
+
+	for _, pkgVar := range validator.typedZarfPackage.Variables {
+		validator.addVarIfNotExists(validatorVar{
+			name:           pkgVar.Name,
+			relativePath:   ".",
+			declaredByUser: true,
+		})
+	}
+
+	for key := range cfg.PkgOpts.SetVariables {
+		validator.addVarIfNotExists(validatorVar{
+			name:           key,
+			relativePath:   ".",
+			declaredByUser: true,
+		})
+	}
+
+	validator.baseDir = cfg.CreateOpts.BaseDir
+
+	if err := lintComponents(&validator, cfg); err != nil {
+		return nil, err
+	}
 
-	lintComponents(&validator, &createOpts)
+	validator.addUnusedVariableErrors()
 
 	if validator.jsonSchema, err = getSchemaFile(); err != nil {
 		return nil, err
@@ -64,15 +88,32 @@ func Validate(createOpts types.ZarfCreateOptions) (*Validator, error) {
 	return &validator, nil
 }
 
-func lintComponents(validator *Validator, createOpts *types.ZarfCreateOptions) {
+func lintComponents(validator *Validator, cfg *types.PackagerConfig) error {
 	for i, component := range validator.typedZarfPackage.Components {
 		arch := config.GetArch(validator.typedZarfPackage.Metadata.Architecture)
 
-		if !composer.CompatibleComponent(component, arch, createOpts.Flavor) {
+		if !composer.CompatibleComponent(component, arch, cfg.CreateOpts.Flavor) {
 			continue
 		}
 
-		chain, err := composer.NewImportChain(component, i, validator.typedZarfPackage.Metadata.Name, arch, createOpts.Flavor)
+		chain, err := composer.NewImportChain(component, i, validator.typedZarfPackage.Metadata.Name, arch, cfg.CreateOpts.Flavor)
+
+		importedVars := chain.MergeVariables([]types.ZarfPackageVariable{})
+		for _, importedVar := range importedVars {
+			validator.addVarIfNotExists(validatorVar{
+				name:             importedVar.Name,
+				declaredByImport: true,
+			})
+		}
+
+		importedConstants := chain.MergeConstants([]types.ZarfPackageConstant{})
+		for _, importedConst := range importedConstants {
+			validator.addVarIfNotExists(validatorVar{
+				name:             importedConst.Name,
+				declaredByImport: true,
+			})
+		}
+
 		baseComponent := chain.Head()
 
 		var badImportYqPath string
@@ -96,16 +137,30 @@ func lintComponents(validator *Validator, createOpts *types.ZarfCreateOptions) {
 		node := baseComponent
 		for node != nil {
 			checkForVarInComponentImport(validator, node)
-			fillComponentTemplate(validator, node, createOpts)
+			fillComponentTemplate(validator, node, &cfg.CreateOpts)
 			lintComponent(validator, node)
+			if err := checkForUnusedVariables(validator, node); err != nil {
+				return err
+			}
 			node = node.Next()
 		}
 	}
+	return nil
+}
+
+func reloadComponentTemplate(component *types.ZarfComponent) error {
+	mappings := map[string]string{}
+	mappings[types.ZarfComponentName] = component.Name
+	err := utils.ReloadYamlTemplate(component, mappings)
+	if err != nil {
+		return err
+	}
+	return nil
 }
 
 func fillComponentTemplate(validator *Validator, node *composer.Node, createOpts *types.ZarfCreateOptions) {
 
-	err := packager.ReloadComponentTemplate(&node.ZarfComponent)
+	err := reloadComponentTemplate(&node.ZarfComponent)
 	if err != nil {
 		validator.addWarning(validatorMessage{
 			description:    err.Error(),

src/pkg/packager/lint/lint_test.go

@@ -167,6 +167,42 @@ func TestValidateSchema(t *testing.T) {
 		require.Equal(t, input, acutal)
 	})
 
+	t.Run("variable is found and marked usedinPackage", func(t *testing.T) {
+		fakeVar := validatorVar{name: "NAME", relativePath: ".", declaredByUser: true, usedByPackage: false}
+		validator := Validator{pkgVars: []validatorVar{fakeVar}}
+		line := "Hello my name is ###ZARF_VAR_NAME###"
+		findVarsInLine(&validator, line, ".")
+		nameVar := validatorVar{name: "NAME", relativePath: ".", declaredByUser: true, usedByPackage: true}
+		require.Len(t, validator.pkgVars, 1)
+		require.Contains(t, validator.pkgVars, nameVar)
+	})
+
+	t.Run("variable is used and package and added to list", func(t *testing.T) {
+		validator := Validator{}
+		line := "deprecated ###ZARF_DATA_INJECTON_MARKER###"
+		findVarsInLine(&validator, line, ".")
+		require.Len(t, validator.findings, 1)
+	})
+
+	t.Run("deprecated variable adds warning", func(t *testing.T) {
+		validator := Validator{}
+		line := "my favorite color is ###ZARF_VAR_COLOR###"
+		findVarsInLine(&validator, line, ".")
+		colorVar := validatorVar{name: "COLOR", relativePath: ".", declaredByUser: false, usedByPackage: true}
+		require.Contains(t, validator.pkgVars, colorVar)
+	})
+
+	t.Run("Do not add the same variable from a different package", func(t *testing.T) {
+		validator := Validator{}
+		originalVar := validatorVar{name: "FAKE_VAR", relativePath: ".", declaredByUser: true, usedByPackage: false}
+		validator.addVarIfNotExists(originalVar)
+		importedVar := validatorVar{name: "FAKE_VAR", relativePath: "fake-path", declaredByUser: false, usedByPackage: false}
+		validator.addVarIfNotExists(importedVar)
+		newVarVal := validatorVar{name: "FAKE_VAR", relativePath: ".", declaredByUser: true, usedByPackage: false}
+		require.Len(t, validator.pkgVars, 1)
+		require.Contains(t, validator.pkgVars, newVarVal)
+	})
+
 	t.Run("Test composable components", func(t *testing.T) {
 		pathVar := "fake-path"
 		unpinnedImage := "unpinned:latest"
@@ -178,7 +214,8 @@ func TestValidateSchema(t *testing.T) {
 				Metadata: types.ZarfMetadata{Name: "test-zarf-package"}}}
 
 		createOpts := types.ZarfCreateOptions{Flavor: "", BaseDir: "."}
-		lintComponents(&validator, &createOpts)
+		cfg := types.PackagerConfig{CreateOpts: createOpts}
+		lintComponents(&validator, &cfg)
 		// Require.contains rather than equals since the error message changes from linux to windows
 		require.Contains(t, validator.findings[0].description, fmt.Sprintf("open %s", filepath.Join("fake-path", "zarf.yaml")))
 		require.Equal(t, ".components.[0].import.path", validator.findings[0].yqPath)