Skip to content

feat: Dependent Resources for External Resources #991

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 19 commits into from
Mar 4, 2022

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Mar 2, 2022

WIP

@csviri csviri self-assigned this Mar 2, 2022
csviri added 11 commits March 2, 2022 15:44
# Conflicts:
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java
@csviri csviri linked an issue Mar 3, 2022 that may be closed by this pull request
@csviri csviri marked this pull request as ready for review March 3, 2022 15:39
@csviri csviri requested a review from metacosm March 3, 2022 15:41
csviri added 2 commits March 4, 2022 10:13
# Conflicts:
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Updater.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
}
return created;
} catch (RuntimeException e) {
if (isEventSourceARecentOperationEventFilter()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the cleanup happen all the time, not only in case of an exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is currently done by handleRecentCreateOrUpdate what can be confusing I agree, will check and change it

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 this, the problem is that it needs to be in the same synchonized block as it is now. Will check once more, but in a separate PR, would rather stick with this now. (when changed the tests started to fail)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you create an issue to track this, please? Let's merge this!


import static io.javaoperatorsdk.operator.processing.dependent.external.PollingDependentResource.DEFAULT_POLLING_PERIOD;

public abstract class PerResourcePollingDependentResource<R, P extends HasMetadata>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have to admit that I'm still confused between this class and PollingDependentResource. Not sure what the differences are and when you'd use one or the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The PerResource polls the supplier for every resource separatelly, the Poller just periodically polls an api regardless of the resources in the caches of the ControllerResourceEventSource

@metacosm
Copy link
Collaborator

metacosm commented Mar 4, 2022

I made small changes that I think improve readability a little. If we can sort out the supply thing, the return values either not being used or returning null vs. Optional and the del method (which I think should stay `delete), we'll be good to merge.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 4, 2022

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 52 Code Smells

40.1% 40.1% Coverage
0.0% 0.0% Duplication

@metacosm metacosm self-requested a review March 4, 2022 16:46
@csviri csviri merged commit 45fb7e0 into next Mar 4, 2022
@csviri csviri deleted the external-dependent-resources branch March 4, 2022 17:41
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.

Utility implementations of DependentResource for External Resources
2 participants