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 search for FeatureGrant in FloatingLicensePackFromRequest #1094

Closed

Conversation

HannesWell
Copy link
Contributor

At the moment FloatingLicensePackFromRequest.forFeature() always returns an empty Optional because it uses a filter that checks if a String is equals to a FeatureRefwhich is always false.

If I understand it right a FloatingLicensePack can have multiple FeatureGrant with the same identifier but different VersionMatch? Therefore I enhanced the equals check to consider that as well when comparing two FeatureRefs.

If forFeature() not always returns an empty Optional featureGrantPeriod() should check if the vivid is set because getVivid() returns a long primitive which is never boxed to null. Filtering out Grants whose vivid is not set ensures that the given default (60) is used instead of zero in that case.

@eparovyshnaya eparovyshnaya self-requested a review June 23, 2022 09:36
@HannesWell HannesWell force-pushed the fixFeatureGrantSearch branch 2 times, most recently from d9dda7a to dd86fc1 Compare July 4, 2022 09:30
@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #1094 (2da7e67) into master (70266f3) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1094      +/-   ##
============================================
- Coverage     33.58%   33.58%   -0.01%     
  Complexity      359      359              
============================================
  Files          1161     1161              
  Lines         25738    25741       +3     
  Branches       1592     1594       +2     
============================================
  Hits           8644     8644              
- Misses        16574    16577       +3     
  Partials        520      520              
Impacted Files Coverage Δ
.../licenses/core/FloatingLicensePackFromRequest.java 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@HannesWell
Copy link
Contributor Author

@eparovyshnaya can you please review this, so we can proceed with this PR?

@eparovyshnaya
Copy link
Contributor

@HannesWell
it looks strange that none of test failed. If the case somehow stayed uncovered, this is the perfect time for such test to appear.

@HannesWell
Copy link
Contributor Author

@HannesWell it looks strange that none of test failed. If the case somehow stayed uncovered, this is the perfect time for such test to appear.

Can you recommand a place and set up for such test-case? At the moment I have no clear idea for that.

@HannesWell HannesWell force-pushed the fixFeatureGrantSearch branch from dd86fc1 to 55bd87e Compare July 19, 2022 10:02
@HannesWell HannesWell force-pushed the fixFeatureGrantSearch branch 2 times, most recently from d8e3274 to 21bfa22 Compare October 12, 2022 08:35
@HannesWell HannesWell force-pushed the fixFeatureGrantSearch branch from 21bfa22 to 775c8a7 Compare October 17, 2022 08:32
@HannesWell HannesWell force-pushed the fixFeatureGrantSearch branch from 775c8a7 to 2da7e67 Compare October 27, 2022 16:36
@HannesWell
Copy link
Contributor Author

@eparovyshnaya can you recommand a place and set up for such test-case?

@ruspl-afed
Copy link
Contributor

ruspl-afed commented Nov 17, 2022

@HannesWell there should be a new test bundle org.eclipse.passage.loc.licenses.core.tests

@eparovyshnaya
Copy link
Contributor

@HannesWell, please make sure your contribution to Passage follows the rules. For start, there must be an issue for the problem and it msut be referenced in each commit for the task.

Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

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

Issue must go first: #1340

Fixed my PR #1341

@HannesWell
Copy link
Contributor Author

Please make sure your contribution to Passage follows the rules. For start, there must be an issue for the problem and it msut be referenced in each commit for the task.

Would you mind to explain why this is a requirement? In the Eclipse top level projects that requirements used to exist when Bugzilla and Gerrit where used, but since all these projects have moved to github an issue is not mandatory anymore, mainly because a PR offers the same possibility as issues and not mandating them makes contributions simpler.

Fixed my PR #1341

Thanks! I'm glad this is finally resolved.

@HannesWell HannesWell deleted the fixFeatureGrantSearch branch April 5, 2024 15:03
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.

3 participants