Skip to content

Primary to seconday dr informer sample #1744

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 8 commits into from
Feb 8, 2023

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Jan 30, 2023

No description provided.

@csviri csviri self-assigned this Jan 30, 2023
@csviri csviri force-pushed the primary-to-seconday-dr-informer-sample branch from 821760a to 075da50 Compare January 30, 2023 14:38
@csviri csviri linked an issue Jan 30, 2023 that may be closed by this pull request
return Map.of(CONFIG_MAP_EVENT_SOURCE, cmES);
}

private String indexKey(String clusterName, String namespace) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be the configMapName instead of clusterName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@metacosm
Copy link
Collaborator

metacosm commented Feb 7, 2023

This should be rebased on next since it includes unrelated changes…

## Condition API Change

In Workflows the target of the condition was the managed resource itself, not a dependent resource. This changed, from
not the API contains the dependent resource.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand that part…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

/**
* Here just simply creating an Event Source, and setting it for the Dependent Resource. Since it
* is not possible to do this setup within the bounds of the KubernetesDependentResource API.
* However, this is quite a corner case; might be covered more out of the box in the future if
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think we should provide a way for dependents to initialize their event sources if needed in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is doable also now just not in an elegant way, and did not want to confuse users. One way it would be just to override this method:

https://github.com/java-operator-sdk/java-operator-sdk/blob/81f7e30ced5bbfe097ca9482e5c7b760ed883320/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java#L47-L50

The SecondaryToPrimary is supported explicitly, but would not be useful to support the PrimaryToSecondary in same way, since for that (as in the example) also access to the context is needed.

We can think about this in the future more, how to cover also this case more elegantly in KubernetesDependentResource, however it's kinda rare case as mentioned. Since not sure if users might want to use dependent resources for these read-only cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use dependent resources everywhere I can, in a declarative way, so I'm all in favor of making everything possible declaratively (as long as we can do it in an elegant way)… 😄

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 thing is that in this cases this is a ready only resource, not reallly reconciled, and that is what dependent resources primary does, so at least it is smelly here a little bit.

Tbh I'm not big fan of annotations, but in this would be good to have possibility to do event sources by annotation as we prototyped in the past - to be consistent at least.

@csviri csviri force-pushed the primary-to-seconday-dr-informer-sample branch from 8556ea0 to 3345435 Compare February 7, 2023 13:57
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 apart some more documentation improvements

@csviri csviri force-pushed the primary-to-seconday-dr-informer-sample branch from 3345435 to 81f7e30 Compare February 8, 2023 08:04
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 8, 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 4 Code Smells

57.4% 57.4% Coverage
0.0% 0.0% Duplication

@csviri csviri merged commit fe820ca into next Feb 8, 2023
@csviri csviri deleted the primary-to-seconday-dr-informer-sample branch February 8, 2023 13:27
csviri added a commit that referenced this pull request Feb 8, 2023
csviri added a commit that referenced this pull request Feb 14, 2023
csviri added a commit that referenced this pull request Feb 23, 2023
csviri added a commit that referenced this pull request Feb 28, 2023
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.

Support PrimaryToSecondaryMapper in KubernetesDependnetResource
2 participants