Skip to content

fix: condition for bulk resources #1688

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

Merged
merged 5 commits into from
Jan 5, 2023
Merged

fix: condition for bulk resources #1688

merged 5 commits into from
Jan 5, 2023

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Jan 2, 2023

Note that this is a temp fix for 4.2. A proper fix with API change will come in 4.3 probably with this PR:
#1690

@csviri
Copy link
Collaborator Author

csviri commented Jan 2, 2023

closes #1687

@csviri
Copy link
Collaborator Author

csviri commented Jan 2, 2023

The test for some reason does not fail. Will investigate also that.

@csviri csviri self-assigned this Jan 2, 2023
@csviri csviri force-pushed the bulk-precondition branch from d83b9f8 to 42a4e17 Compare January 2, 2023 09:59
@csviri csviri changed the title feat: condition for bulk resources fix: condition for bulk resources Jan 2, 2023
@csviri csviri marked this pull request as ready for review January 2, 2023 10:07
@csviri csviri requested a review from metacosm January 2, 2023 10:09
return condition.map(c -> c.isMet(primary,
dependentResource.getSecondaryResource(primary, context).orElse(null),
context))
(R) resources, context))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't work because R can never be a Map in a DependentResource declaration so this should most likely cause a ClassCastException.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That thing does not get into the bytecode, but also there is a test for this in the PR. Will try to make it more nice though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked but this is still the simplest way to do locally, this will change probably within this issue:
#1689

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will improve this sample so it shows more the issue with the Bulk resources.

@csviri csviri requested a review from metacosm January 3, 2023 09:25
@csviri
Copy link
Collaborator Author

csviri commented Jan 3, 2023

@metacosm improved the test, commented it, would be quite unefficient to do a real life example, but I think here the main point is the API to show in the IT.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

77.8% 77.8% Coverage
0.0% 0.0% Duplication

@csviri csviri linked an issue Jan 4, 2023 that may be closed by this pull request
Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

LGTM after we make it clearer that this is a temporary fix to be addressed better in #1690

@csviri csviri merged commit c4fc798 into main Jan 5, 2023
@csviri csviri deleted the bulk-precondition branch January 5, 2023 09:52
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.

Managed BulkDependentResource fails with reconcilePrecondition
2 participants