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

Unique validator for packaging metadata (BugFix) #920

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

fernando79513
Copy link
Collaborator

@fernando79513 fernando79513 commented Jan 9, 2024

Description

This PR is a follow up of #909. We were missing a check during the validation phase to make sure that there are not repeated packages in the packaging metadata files.

For that purpose, we have used the UniqueValueValidator for the Depends, Recommends and Suggests fields of the packaging metadata units, so they can't be repeated.

This change is not probable to make any break in previous versions, but it is possible if some package metadata units were using the same package twice.

Resolved issues

Closes CHECKBOX-1062

Documentation

Updated packaging-meta-data.rst

Tests

To run the new packaging tests:

cd ~/checkbox/checkbox-ng
python3 -m pytest -k PackagingUnitFieldValidationTests

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1cf70e8) 37.50% compared to head (df6635b) 37.59%.
Report is 5 commits behind head on main.

Files Patch % Lines
checkbox-ng/plainbox/impl/unit/packaging.py 94.00% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
+ Coverage   37.50%   37.59%   +0.09%     
==========================================
  Files         313      313              
  Lines       34809    34835      +26     
  Branches     5983     5987       +4     
==========================================
+ Hits        13055    13097      +42     
+ Misses      21174    21154      -20     
- Partials      580      584       +4     
Flag Coverage Δ
checkbox-ng 63.26% <94.11%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kissiel
Copy link
Contributor

kissiel commented Jan 10, 2024

This change is not probable to make any break in previous versions, but it is possible if some package metadata units were using the same package twice.

So we went from unpredictable behavior, to a predictable one. Having two units like that is/was a problem, so if we're breaking something with your changes, we really are just shedding light on the problem. I wouldn't go as far as calling it a breaking change.

@fernando79513
Copy link
Collaborator Author

This change is not probable to make any break in previous versions, but it is possible if some package metadata units were using the same package twice.

So we went from unpredictable behavior, to a predictable one. Having two units like that is/was a problem, so if we're breaking something with your changes, we really are just shedding light on the problem. I wouldn't go as far as calling it a breaking change.

Okey, I will change it to "BugFix" then

@fernando79513 fernando79513 changed the title Unique validator for packaging metadata (Breaking) Unique validator for packaging metadata (BugFix) Jan 10, 2024
Check if a packaging metadata unit contains the same package name
during the validation step
@fernando79513 fernando79513 force-pushed the unique-validator-for-packaging-metadata branch from a74cd0d to df6635b Compare January 10, 2024 15:11
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

If I understand the code correctly, the "Depends" field in the meta were capitalized because they are one-offs (type-like). But the code proposed here adds properties based on that section name (Capitalized), so the field name should be lowercase.

checkbox-ng/plainbox/impl/unit/packaging.py Show resolved Hide resolved
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations. This warrants a +1 now.

@kissiel kissiel merged commit 5766167 into main Jan 12, 2024
29 checks passed
@kissiel kissiel deleted the unique-validator-for-packaging-metadata branch January 12, 2024 12:54
LiaoU3 pushed a commit to LiaoU3/checkbox that referenced this pull request Mar 20, 2024
* Added validation check for same package

Check if a packaging metadata unit contains the same package name
during the validation step

* Updated documentation

* Increased test coverage
binli pushed a commit to binli/checkbox that referenced this pull request Mar 22, 2024
* Added validation check for same package

Check if a packaging metadata unit contains the same package name
during the validation step

* Updated documentation

* Increased test coverage
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.

2 participants