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

Use JcrPackageDefinition for retrieving metadata #3227

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Dec 4, 2023

This is less error prone and faster than acting on the compressed underlying ZIP binary
Always close JcrPackageManager (although it should now no longer be necessary)
Some refactoring

This closes #3225

This is less error prone and faster than acting on the compressed
underlying ZIP binary
Always close JcrPackageManager (although it should now no longer be
necessary)
Some refactoring

This closes #3225
Copy link

codecov bot commented Dec 4, 2023

Codecov Report

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

Comparison is base (6649768) 1.34% compared to head (b3869e6) 1.34%.

Files Patch % Lines
...egarbagecollector/PackageGarbageCollectionJob.java 0.00% 46 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             master   #3227   +/-   ##
========================================
  Coverage      1.34%   1.34%           
  Complexity       44      44           
========================================
  Files           721     721           
  Lines         29537   29515   -22     
  Branches       3844    3837    -7     
========================================
  Hits            397     397           
+ Misses        29124   29102   -22     
  Partials         16      16           

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

&& otherPackageId.get().getName().equals(thisPackageId.get().getName());
private boolean isLatestInstalled(JcrPackageDefinition referencePkgDefinition, Stream<JcrPackage> installedPackages) throws RepositoryException {
try {
Optional<@Nullable JcrPackageDefinition> lastInstalledPckDefinitionOptional = installedPackages
Copy link
Contributor

@joerghoh joerghoh Dec 5, 2023

Choose a reason for hiding this comment

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

that @Nullable annotation seems a bit odd to me. Isn't Optional<> just covering the nullable case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that was a leftover from the refactoring. Fixed now.

try {
return p.getDefinition();
} catch (RepositoryException e) {
throw new UncheckedRepositoryException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this exception contain enough metadata to identify the package where the exception happened? Or should we add an additional message to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now enrich the relevant exception with the package path

@kwin kwin requested a review from joerghoh December 5, 2023 12:44
Copy link
Contributor

@joerghoh joerghoh left a comment

Choose a reason for hiding this comment

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

lgtm

@davidjgonzalez davidjgonzalez merged commit 7b542e0 into master Jan 16, 2024
19 of 21 checks passed
@davidjgonzalez davidjgonzalez deleted the bugfix/pck-garbage-collector-use-definitions branch January 19, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PackageGarbageCollector leaves temp files behind
3 participants