-
Notifications
You must be signed in to change notification settings - Fork 233
feat: ResourceIDMapper for external event sources, external dependents and bulk dependents #3020
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
base: next
Are you sure you want to change the base?
Conversation
…pper Signed-off-by: Attila Mészáros <[email protected]>
Will check if this could be elegantly extended to bulk resources Signed-off-by: Attila Mészáros <[email protected]>
…s and bulk dependents 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]>
docs/content/fileList.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a deletion of empty file, it is not needed.
...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDMapper.java
Outdated
Show resolved
Hide resolved
...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDMapper.java
Outdated
Show resolved
Hide resolved
...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDMapper.java
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Outdated
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Outdated
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Outdated
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Show resolved
Hide resolved
…tor/processing/ResourceIDMapper.java Co-authored-by: Martin Stefanko <[email protected]>
…tor/processing/ResourceIDMapper.java Co-authored-by: Martin Stefanko <[email protected]>
…tor/processing/ResourceIDProvider.java Co-authored-by: Martin Stefanko <[email protected]>
…tor/processing/ResourceIDProvider.java Co-authored-by: Martin Stefanko <[email protected]>
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]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small stuff, otherwise good
docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Chris Laprun <[email protected]>
Co-authored-by: Martin Stefanko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several comments:
- As previously mentioned, I don't think that putting the responsibility of providing the external resource id on the external resource class is a good idea. We need to decide which design to go with now because this is already a breaking change and it shouldn't change again soon after, if we decide that putting the id "generation" on the dependent class (for example) is a better design later.
- The documentation for this feature is rather lacking and doesn't really explain the concepts or how to use things so it needs to be improved, imo.
That is completely optional, but allows us to add meaningful default implementation for the mapper. What alternative would you propose?
Not shure what do you mean by this. Can you pls elaborate?
Yeah, in general we have quite badly documented, features around it, would rather followup with a comprehensive rework on the docs. |
it is by definition single, so namespace scoping is out of the picture, also does not confuse this way with some hidden intention in the background Signed-off-by: Attila Mészáros <[email protected]>
No description provided.