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

feat: optional dependent resource #2240

Closed
wants to merge 54 commits into from
Closed

feat: optional dependent resource #2240

wants to merge 54 commits into from

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Feb 13, 2024

No description provided.

@csviri csviri self-assigned this Feb 13, 2024
@csviri csviri linked an issue Feb 13, 2024 that may be closed by this pull request
@csviri csviri force-pushed the optional-dependent branch from 8c0b66b to a308146 Compare March 6, 2024 09:50
@csviri csviri marked this pull request as ready for review March 6, 2024 14:32
@openshift-ci openshift-ci bot requested review from adam-sandor and andreaTP March 6, 2024 14:32
@csviri
Copy link
Collaborator Author

csviri commented Mar 6, 2024

For customizing the expiration mechanism there will be a separate PR.

@csviri csviri requested a review from metacosm March 6, 2024 15:05
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2024
@csviri csviri force-pushed the optional-dependent branch from 17004b3 to cdce182 Compare March 12, 2024 09:18
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2024
package io.javaoperatorsdk.operator.processing.expiration;


public interface Expiration {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need an interface for this? Do we envision multiple implementations?

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 same principle as for retry, some might be able different strategy, so created an abstraction for that.

@@ -0,0 +1,9 @@
package io.javaoperatorsdk.operator.processing.expiration;

public interface ExpirationExecution {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment than for Expiration.

import io.javaoperatorsdk.operator.processing.retry.GenericRetry;
import io.javaoperatorsdk.operator.processing.retry.Retry;

public class CRDPresentActivationCondition implements Condition<HasMetadata, 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 think this should be better implemented as a utilty class that could be reused elsewhere and shared across reconcilers, in particular because Controller also can check the availability of the CRD so it would be better if the same logic / implementation / cache could be reused/shared everywhere this functionality is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is true, I don't like utilities / static scoped states, usually, those lead to problems (eventually). But will think about how to handle that.

@csviri csviri force-pushed the optional-dependent branch from cdce182 to 422224d Compare March 19, 2024 07:34
@csviri csviri force-pushed the optional-dependent branch from 422224d to daf2262 Compare March 26, 2024 14:19
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024
@csviri csviri force-pushed the next branch 2 times, most recently from 88c4960 to afe3d7d Compare May 17, 2024 10:46
csviri and others added 18 commits May 21, 2024 21:02
Signed-off-by: Attila Mészáros <[email protected]>
…2331)

Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Chris Laprun <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Chris Laprun <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Chris Laprun <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Chris Laprun <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
* improve: ensure unique name on event sources

Signed-off-by: Attila Mészáros <[email protected]>

* fix

Signed-off-by: Attila Mészáros <[email protected]>

---------

Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
@csviri csviri force-pushed the optional-dependent branch from 7f97e23 to 2d22dd2 Compare June 13, 2024 07:26
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2024
Signed-off-by: Attila Mészáros <[email protected]>
@openshift-merge-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2024
@csviri csviri closed this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for optional @Dependent resources
3 participants