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 reasoning about server semantic version #174

Merged
merged 10 commits into from
Apr 5, 2024
Merged

Conversation

bdpedigo
Copy link
Collaborator

@bdpedigo bdpedigo commented Mar 27, 2024

Note: Currently, this is just for the chunkedgraph, but could easily be extended to the other services if we like the pattern

This PR adds

  • Adds logic for talking to an endpoint which just returns the full semver for the service
    • Happens on sub-client creation
  • Stores this info as a Version object, which allows for easy operations like checking >,>= etc.
  • Adds a decorator for convenience, which allows specification that a method or its keyword arguments are only valid for specific constraints of server version
    • Opted not to do that for arguments, could discuss further but since those are required that felt like it didn't fit the pattern
  • As an example, applies this decorator to a recent addition to the level2_chunk_graph bounds argument

This PR does not add

A few things we discussed but I think are separate PRs

  • In theory, this kind of reasoning about the server version might allow for consolidating the multiple API version clients into one for each service, with method-level logic about dispatching to particular sub-functions depending on the version
  • There are some use cases that don't fall under the decorator that are more adhoc, for instance, the recent bug where using timestamp and select columns can be unsafe. I think those will need to be done on a case by case basis, but could at least use some of the tools here

Worth discussing

  • How to generalize testing with this kind of thing (since mocking needs to happen @ client level)
  • How to handle case for servers that don't have this endpoint yet (I have some ideas here but it gets ugly)
    • How to refactor to use handle_response but be OK with a 404 error

@bdpedigo bdpedigo changed the title [DRAFT] Add support for reasoning about server semantic version Add support for reasoning about server semantic version Mar 28, 2024
@bdpedigo bdpedigo requested a review from fcollman April 3, 2024 16:19
@fcollman fcollman merged commit 4200850 into master Apr 5, 2024
16 checks passed
@bdpedigo bdpedigo deleted the semver branch April 5, 2024 21:52
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.

2 participants