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(pom): recursive check all nested depManagements with import scope #285

Merged

Conversation

DmitriyLewen
Copy link
Collaborator

@DmitriyLewen DmitriyLewen commented Jan 18, 2024

Description

This PR corrects 2 related problems:

  • we need to check all nested depManagements using import (recursively running resolveDepManagement).
  • exclude shadow overwriting of props when some iterations of nested depManagements use the same props (e.g., ${project.version}).

See updated import dependencyManagement test for more details.

Related Issues

@DmitriyLewen DmitriyLewen self-assigned this Jan 18, 2024
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Do we need this level nest? IIUC, the new test is importing pom as below.

  • import-dependency-management/pom.xml
    • example-dependency-management@3.3.3
      • example-dependency-management2@3.3.3
        • example-dependency-management3@2.2.2

I'm wondering if a two-level nest is enough.

  • import-dependency-management/pom.xml
    • example-dependency-management@3.3.3
      • example-dependency-management2@1.1.1

e.g. knqyf263@b8d492f

I'm probably missing something. Please let me know what we want to test with the test case.

@DmitriyLewen
Copy link
Collaborator Author

These 3 nested depManagements and dependency tag for 1 of them are required to show the need of clone map properties.

I found this problem after adding recurcive - we overwrite the properties in the shadow.

@knqyf263
Copy link
Collaborator

Reverting your change failed the test, meaning the simplified test can catch the edge case you found, right?

@knqyf263
Copy link
Collaborator

knqyf263 commented Jan 31, 2024

BTW, I wanted to push the commits for testing to my fork, but pushed them to your form by mistake. Sorry 😔

@DmitriyLewen
Copy link
Collaborator Author

Reverting your change failed the test, meaning the simplified test can catch the edge case you found, right?

I updated PR description to better understand fixes in this PR.

I created similar of your test first(f7ec709). But after investigation more i found second problem (overwrite props).

Your test only checks first fix (nested depManagements).
But if you revert these changes (shadow overwrite props) you still pass test.

BTW, I wanted to push the commits for testing to my fork, but pushed them to your form by mistake. Sorry 😔

No problem. I will revert these changes when we are done discussing these fixes.

Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263 knqyf263 force-pushed the fix-pom/nested-import-dep-managements branch from 226facd to 7d768df Compare January 31, 2024 11:29
@knqyf263
Copy link
Collaborator

Thanks. I now understand what it was testing. As per your explanation, I've added properties causing the overriding bug. Could you take a look?

@DmitriyLewen
Copy link
Collaborator Author

DmitriyLewen commented Jan 31, 2024

I was so focused on example from aquasecurity/trivy#5899 that I didn't even think to just declare new property 😄

This is what i wanted to check in my test.
Thank you for making this test easier!

@knqyf263 knqyf263 merged commit 2779e24 into aquasecurity:main Jan 31, 2024
2 checks passed
@knqyf263
Copy link
Collaborator

Thanks for confirming.

@knqyf263
Copy link
Collaborator

I was so focused on example from aquasecurity/trivy#5899 that I didn't even think to just declare new property 😄

I wasn't part of that discussion, so I was able to focus on the essential issue 👍

@DmitriyLewen DmitriyLewen deleted the fix-pom/nested-import-dep-managements branch February 1, 2024 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants