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.

Sign up for GitHub

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

Jump to bottom

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

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

merged 16 commits into from
Feb 6, 2025

Conversation

AustinAbro321
Copy link
Contributor

@AustinAbro321 AustinAbro321 commented Feb 3, 2025 •
edited
Loading

Description

Fixes #3457

Checklist before merging

@AustinAbro321
inital example showing failure
e954a3e 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
minimal example
0c98821 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
technically work, but very messy
42063dc 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
tests passing
6e6d983 
Signed-off-by: Austin Abro <[email protected]>
@netlify Netlify
Copy link

netlify bot commented Feb 3, 2025 •
edited
Loading

✅ Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 837f603
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/67a37e6fc72f7f00078d9475

@codecov Codecov
Copy link

codecov bot commented Feb 3, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/internal/packager2/layout/import.go 44.05% <100.00%> (+4.90%) ⬆️

@AustinAbro321
double got rid of components
f5297d4 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
tests passing
a91c1d0 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
update tests
da40ad6 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
add test for variables
a0f7fef 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321 AustinAbro321 changed the title Fix import false positives when two different packages import each other with different components Fix import false positives when two packages import each other on separate chains Feb 3, 2025
@AustinAbro321
add better tests cases
8995af6 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321 AustinAbro321 changed the title Fix import false positives when two packages import each other on separate chains fix: avoid false positive cyclic chain error when two packages import each other on separate component chains Feb 3, 2025
@AustinAbro321 AustinAbro321 marked this pull request as ready for review February 4, 2025 14:15
@AustinAbro321 AustinAbro321 requested review from a team as code owners February 4, 2025 14:15
@AustinAbro321
make comment more sensible
06b130f 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
comment
cb7b177 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321
stack
91eeeea 
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321 AustinAbro321 changed the title fix: avoid false positive cyclic chain error when two packages import each other on separate component chains fix: avoid incorrect cyclic error when two packages import each other on separate component chains Feb 4, 2025
mkcp
mkcp approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@mkcp mkcp left a comment

Choose a reason for hiding this comment

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

Stepped through everything and LGTM 👍

src/internal/packager2/layout/import.go
// 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.

👍

@AustinAbro321 AustinAbro321 added this pull request to the merge queue Feb 6, 2025
@AustinAbro321 AustinAbro321 added this to the v0.49.0 milestone Feb 6, 2025
Merged via the queue into main with commit e5a9d91 Feb 6, 2025
26 checks passed
@AustinAbro321 AustinAbro321 deleted the fix-import-false-positives branch February 6, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkcp mkcp mkcp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.49.0
Development

Successfully merging this pull request may close these issues.

cyclic imports detection logic evaluates components that are not included in the package
2 participants
@AustinAbro321 @mkcp