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

Open
wants to merge 43 commits into
base: next
Choose a base branch
from
Open

feat: optional dependent resource #2240

wants to merge 43 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 marked this pull request as ready for review 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 and others added 15 commits March 11, 2024 20:18
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
* feat: move name is directly to dependent resource

- use this name when throwing aggregate exception

Signed-off-by: Attila Mészáros <csviri@gmail.com>

* refactor to use a dedicated interface for setting the name

Signed-off-by: Attila Mészáros <csviri@gmail.com>

* refactor: add default implementation for name() (#2255)

Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>

---------

Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
@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.

metacosm and others added 17 commits March 20, 2024 12:20
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>

Signed-off-by: Attila Mészáros <csviri@gmail.com>

Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri and others added 3 commits March 26, 2024 16:11
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
@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
@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/test-infra repository.

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