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

v2 endpoint functions in _endpointsv2.py #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlexMercedCoder
Copy link

in the file, _endpointsv2.py I created versions of all the endpoint methods that apply to the v2 endpoints (tried to stick as closely to the style that the originals were written in), for review before the data classes for schemas that are different in v2, then the client class can be worked on .

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

Looks good so far, added a few suggestions.

I think, it's better to continue in this PR until it's complete incl tests.

For the NessieClient class: WDYT about making that one "abstract" and move the existing functionality to a NessieClientV1 and the V2 stuff into a NessieClientV2? In NessieClient a "static function" (not sure if that exists in Python tho), that can construct a V1 or V2 client based on the configuration.

In the Java client we have a mechanism that checks that the API versions match (correct endpoint URL). In the Java client it "kicks in" right before the first request is issued - it performs a "GET /config" and inspects the "raw JSON" response, so it's independent from "serialization constraints" - see org.projectnessie.client.http.NessieApiCompatibilityFilter#check.



## Get to /trees/{ref}/history
def reflogv2(
Copy link
Member

Choose a reason for hiding this comment

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

Reflog can be completely omitted. It's already deprecated and not supported in V2.

# limitations under the License.
#

"""Direct API operations on Nessie with requests."""
Copy link
Member

Choose a reason for hiding this comment

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

Note that V2 clients should send a Nessie-Client-Spec: 2 header (this int value might change/increase) to benefit from certain improvements.
Currently(!) that's only the error-details field (org.projectnessie.error.NessieErrorDetails) in org.projectnessie.error.NessieError#getErrorDetails.

# limitations under the License.
#

"""Direct API operations on Nessie with requests."""
Copy link
Member

Choose a reason for hiding this comment

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

Guess it's easier to have these functions in a class with fields for:

  • client-spec (int)
  • auth
  • ssl_verify
  • timeout_sec
  • base_url

and have a constructor that takes these parameters, maybe directly (or additionally) from the confuse-config object.

If that's a separate class, the _v2 suffixes can go away as well.

@AlexMercedCoder
Copy link
Author

Looks good so far, added a few suggestions.

I think, it's better to continue in this PR until it's complete incl tests.

For the NessieClient class: WDYT about making that one "abstract" and move the existing functionality to a NessieClientV1 and the V2 stuff into a NessieClientV2? In NessieClient a "static function" (not sure if that exists in Python tho), that can construct a V1 or V2 client based on the configuration.

In the Java client we have a mechanism that checks that the API versions match (correct endpoint URL). In the Java client it "kicks in" right before the first request is issued - it performs a "GET /config" and inspects the "raw JSON" response, so it's independent from "serialization constraints" - see org.projectnessie.client.http.NessieApiCompatibilityFilter#check.

Should I try a seperate branch for soley the implementation of the of the abstract class, get that merged in then modify these methods. Just cause I sporadically have to time to work on this so I don't want too much being held up between these points.

@snazy
Copy link
Member

snazy commented Jul 31, 2023

Should I try a seperate branch for soley the implementation of the of the abstract class, get that merged in then modify these methods. Just cause I sporadically have to time to work on this so I don't want too much being held up between these points.

It's fine to use multiple commits, if necessary, in this PR (and branch) for now. Once we have the API settled and tests for the new code, we can merge it.

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