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

Added support to other naming strategy #754

Merged

Conversation

eliax1996
Copy link
Contributor

@eliax1996 eliax1996 commented Nov 3, 2023

Before that commit the other strategies where existing but not effectively usable/tested in the codebase.
This pr adds the support and set tests to effectively test the naming strategies.
During the development I've also added a little bit more typing over the files I've touched to improve the codebase

@eliax1996 eliax1996 force-pushed the eliax1996/general-refactor-of-different-components branch 2 times, most recently from 3221250 to b90af7b Compare November 3, 2023 10:08
@eliax1996 eliax1996 marked this pull request as ready for review November 3, 2023 10:09
@eliax1996 eliax1996 requested review from a team as code owners November 3, 2023 10:09
Copy link
Contributor

@tvainika tvainika left a comment

Choose a reason for hiding this comment

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

See inline the fundamental question how record name strategy should work to be clarified

karapace/config.py Outdated Show resolved Hide resolved
karapace/serialization.py Show resolved Hide resolved
tests/unit/test_serialization.py Outdated Show resolved Hide resolved
karapace/serialization.py Outdated Show resolved Hide resolved
@eliax1996 eliax1996 force-pushed the eliax1996/general-refactor-of-different-components branch 3 times, most recently from af7f346 to 3471482 Compare November 3, 2023 10:52
@eliax1996 eliax1996 force-pushed the eliax1996/general-refactor-of-different-components branch from 3471482 to 462d3b7 Compare November 7, 2023 16:35
@eliax1996 eliax1996 force-pushed the eliax1996/general-refactor-of-different-components branch 2 times, most recently from bc0bfd7 to a7d0843 Compare November 7, 2023 16:53
@eliax1996 eliax1996 dismissed tvainika’s stale review November 8, 2023 13:32

please take a second look :)

Copy link
Contributor

@giuseppelillo giuseppelillo left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Could you also add a test on how the record and topic-record naming strategy are supposed to work with an Avro schema without namespace?

Copy link
Contributor

@tvainika tvainika left a comment

Choose a reason for hiding this comment

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

LGTM except documentation change is not correct...

README.rst Outdated Show resolved Hide resolved
@eliax1996 eliax1996 force-pushed the eliax1996/general-refactor-of-different-components branch from a7d0843 to 952b9f4 Compare November 8, 2023 16:38
@eliax1996 eliax1996 dismissed tvainika’s stale review November 8, 2023 16:38

addressed requested changes

@eliax1996 eliax1996 force-pushed the eliax1996/general-refactor-of-different-components branch from 952b9f4 to 74b4cc9 Compare November 8, 2023 16:43
tvainika
tvainika previously approved these changes Nov 9, 2023
@eliax1996 eliax1996 force-pushed the eliax1996/general-refactor-of-different-components branch from 74b4cc9 to 535d652 Compare November 9, 2023 09:25
@giuseppelillo giuseppelillo merged commit 0a57f3b into main Nov 9, 2023
8 checks passed
@giuseppelillo giuseppelillo deleted the eliax1996/general-refactor-of-different-components branch November 9, 2023 09:39
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.

3 participants