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

yarn classic: Offline mirror collision #786

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

slimreaper35
Copy link
Member

@slimreaper35 slimreaper35 commented Jan 17, 2025

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

@eskultety
Copy link
Member

Based on the commit message (and a missing issue) it is difficult to understand why we're trying to provide a hotfix for a problem that at first glance appears to be a project configuration error, so I'd suggest creating an issue first with all the necessary context and a reproducer to shed more light on this PR since the commit message also hints at a genuine bug in our tool.

@slimreaper35 slimreaper35 changed the title yarn classic: Drop offline mirror collision exception raise yarn classic: Offline mirror collision Jan 20, 2025
@slimreaper35
Copy link
Member Author

I changed the logic completely by disabling exception raise only for two identical registry packages. That should not be a problem in my opinion. I would finish this problem here and now, rather than just create a temporary solution with a promise of a proper fix in the future.

@eskultety
Copy link
Member

I would finish this problem here and now, rather than just create a temporary solution with a promise of a proper fix in the future.

Heavy +1 to this! I'll have a look some time this week (earlier rather than later).

cachi2/core/package_managers/yarn_classic/main.py Outdated Show resolved Hide resolved
tests/unit/package_managers/yarn_classic/test_main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn_classic/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn_classic/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn_classic/main.py Outdated Show resolved Hide resolved
@brunoapimentel
Copy link
Contributor

So, just to clarify my understanding, I'll paste an example when processing kiali. The strip-ansi-6.0.1 tarball is duplicated in that repo:

[RegistryPackage(url='https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9', name='strip-ansi', version='6.0.1', integrity='sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==', dev=True),

RegistryPackage(url='https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9', name='strip-ansi', version='6.0.1', integrity='sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==', dev=True)]

We're assuming it is safe because we realize it is exactly the same tarball, source url and hash included. Even if we had a Github tarball, for example, we would still verify all the details to assume this "collison" was safe (that is, we are downloading the exact tarball from the same place twice).

IIUIC, this solution LGTM. I'd just expand the docstring of the function to make this behavior clear, I'll certainly forget it soon 😅

@slimreaper35 slimreaper35 marked this pull request as ready for review January 24, 2025 13:45
@slimreaper35
Copy link
Member Author

IIUIC, this solution LGTM. I'd just expand the docstring of the function to make this behavior clear, I'll certainly forget it soon 😅

Added more descriptive docstring.

@brunoapimentel
Copy link
Contributor

/re-test

@slimreaper35
Copy link
Member Author

/retest

Replace the previous duplicate tarball check with a more accurate collision
detection mechanism. The new implementation verifies that all packages mapped
to the same tarball name in the offline mirror are actually identical.

The previous Counter-based approach only checked for duplicate filenames
without validating package equivalence. The new defaultdict-based solution
properly detects cases where different packages would overwrite each other's
tarballs in the mirror.

Update test cases to cover valid duplicate scenarios (identical packages) and
invalid collisions (different packages with same tarball name).

Signed-off-by: Michal Šoltis <[email protected]>
@eskultety eskultety added this pull request to the merge queue Jan 29, 2025
Merged via the queue into containerbuildsystem:main with commit 1c4c6ea Jan 29, 2025
15 of 16 checks passed
@eskultety eskultety deleted the yarn branch January 29, 2025 08:32
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.

Offline mirror collision with npm aliased package name
4 participants