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

Add support for '-1' as version #669

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mattieserver
Copy link

About this change - What it does

This allows adding '-1' as the version just like 'latest'.

References: #668

Why this way

This seems like the quickest way to add support for the '-1'

@mattieserver mattieserver requested review from a team as code owners June 30, 2023 13:32
Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

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

This is the easiest way to add the -1 support. I'd prefer to move the responsibility of resolving to a Version class and the validation of the input string closer to the REST API. That would mean a bit more of refactoring:

  • Move karapace.typing.Version to karapace.schema_models.Version
  • Change to a class and implement the resolving and validation.
  • Change the _resolve_version calls version.resolve(schema_versions).
  • Create the version objects from the input string in karapace.schema_registry_api.
  • Add tests

@mattieserver
Copy link
Author

I expected something like this 😄

I try to implement the changes like you requested.

@mattieserver mattieserver marked this pull request as draft July 25, 2023 11:50
@aiven-anton
Copy link
Contributor

This should possibly be kept in sync with the respective handling here:

reference = reference.resolve(max(subject_data))

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.

None yet

3 participants