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

fix: avoid incorrect cyclic error when two packages import each other on separate component chains #3460

Merged
merged 16 commits into from
Feb 6, 2025
Merged
24 changes: 20 additions & 4 deletions src/internal/packager2/layout/import.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,19 @@ import (
"github.com/zarf-dev/zarf/src/pkg/zoci"
)

func getComponentToImportName(component v1alpha1.ZarfComponent) string {
if component.Import.Name != "" {
return component.Import.Name
}
return component.Name
}

func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath, arch, flavor string, importStack []string) (v1alpha1.ZarfPackage, error) {
// Zarf imports merge in the top level package objects variables and constants
// however, imports are defined at the component level.
// Two packages can both import one another as long as the importing components are on a different chains.
// To detect cyclic imports, the stack is checked to see if the package has already been imported on that chain.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// Recursive calls only include components from the imported pkg that have the name of the component to import
importStack = append(importStack, packagePath)

variables := pkg.Variables
Expand Down Expand Up @@ -61,6 +73,13 @@ func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath,
if err != nil {
return v1alpha1.ZarfPackage{}, err
}
var relevantComponents []v1alpha1.ZarfComponent
for _, importedComponent := range importedPkg.Components {
if importedComponent.Name == getComponentToImportName(component) {
relevantComponents = append(relevantComponents, importedComponent)
}
}
importedPkg.Components = relevantComponents
importedPkg, err = resolveImports(ctx, importedPkg, importPath, arch, flavor, importStack)
if err != nil {
return v1alpha1.ZarfPackage{}, err
Expand All @@ -80,10 +99,7 @@ func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath,
}
}

name := component.Name
if component.Import.Name != "" {
name = component.Import.Name
}
name := getComponentToImportName(component)
found := []v1alpha1.ZarfComponent{}
for _, component := range importedPkg.Components {
if component.Name == name && compatibleComponent(component, arch, flavor) {
Expand Down
49 changes: 35 additions & 14 deletions src/internal/packager2/layout/import_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,47 @@ func TestResolveImportsCircular(t *testing.T) {
require.EqualError(t, err, "package testdata/import/circular/second imported in cycle by testdata/import/circular/third in component component")
}

func TestResolveImportsBranches(t *testing.T) {
func TestResolveImports(t *testing.T) {
t.Parallel()
ctx := testutil.TestContext(t)
lint.ZarfSchema = testutil.LoadSchema(t, "../../../../zarf.schema.json")
testCases := []struct {
name string
path string
}{
{
name: "two zarf.yaml files import each other",
path: "./testdata/import/import-each-other",
},
{
name: "variables and constants are resolved correctly",
path: "./testdata/import/variables",
},
{
name: "two separate chains of imports importing a common file",
path: "./testdata/import/branch",
},
}

// Get the parent package
b, err := os.ReadFile(filepath.Join("./testdata/import/branch", ZarfYAML))
require.NoError(t, err)
pkg, err := ParseZarfPackage(b)
require.NoError(t, err)
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

resolvedPkg, err := resolveImports(ctx, pkg, "./testdata/import/branch", "", "", []string{})
require.NoError(t, err)
b, err := os.ReadFile(filepath.Join(tc.path, ZarfYAML))
require.NoError(t, err)
pkg, err := ParseZarfPackage(b)
require.NoError(t, err)

// ensure imports were resolved correctly
b, err = os.ReadFile(filepath.Join("./testdata/import/branch", "expected.yaml"))
require.NoError(t, err)
expectedPkg, err := ParseZarfPackage(b)
require.NoError(t, err)
require.Equal(t, expectedPkg, resolvedPkg)
resolvedPkg, err := resolveImports(ctx, pkg, tc.path, "", "", []string{})
require.NoError(t, err)

b, err = os.ReadFile(filepath.Join(tc.path, "expected.yaml"))
require.NoError(t, err)
expectedPkg, err := ParseZarfPackage(b)
require.NoError(t, err)
require.Equal(t, expectedPkg, resolvedPkg)
})
}
}

func TestValidateComponentCompose(t *testing.T) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ components:
- name: common
description: This gets imported by the parent and child directly
required: true
- name: parent-child-common
- name: parent-importing-child
description: This gets imported in a chain parent->child->common
required: true
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ components:
import:
path: common

- name: parent-child-common
- name: parent-importing-child
required: true
import:
name: parent-child-common
path: child
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
kind: ZarfPackageConfig
metadata:
name: parent-package
components:
- name: first-is-parent
description: this component gets imported by second-package
- name: second-is-parent
description: this component gets imported by first-package
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
kind: ZarfPackageConfig
metadata:
name: first-package

components:
- name: first-is-parent
description: "this component gets imported by second-package"

- name: second-is-parent
description: "this component imports second-package"
import:
path: ../second-import
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
kind: ZarfPackageConfig
metadata:
name: second-package

components:
- name: first-is-parent
description: "this component imports first-package"
import:
path: ../first-import

- name: second-is-parent
description: "this component gets imported by first-package"
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
kind: ZarfPackageConfig

metadata:
name: parent-package

components:
- name: first-is-parent
import:
path: first-import

- name: second-is-parent
import:
path: second-import
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
kind: ZarfPackageConfig
metadata:
name: parent-package
constants:
- name: PARENT_CONSTANT
value: "value from parent"
- name: CHILD_CONSTANT
value: "value from child"
variables:
- name: PARENT_VAR
value: "value from parent"
- name: CHILD_VAR
value: "value from child"
components:
- name: component-to-test-vars
required: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
kind: ZarfPackageConfig
metadata:
name: parent-package

constants:
- name: PARENT_CONSTANT
value: "value from child"
- name: CHILD_CONSTANT
value: "value from child"
variables:
- name: PARENT_VAR
value: "value from child"
- name: CHILD_VAR
value: "value from child"
components:
- name: component-to-test-vars
required: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
kind: ZarfPackageConfig
metadata:
name: parent-package

constants:
- name: PARENT_CONSTANT
value: "value from parent"
variables:
- name: PARENT_VAR
value: "value from parent"
components:
- name: component-to-test-vars
required: true
import:
path: import
Loading