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 new GCS collection doc versions #675

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Jan 30, 2023

Support versions '1.6.0' and '1.7.0' via guest_auth_policy_id and long user_message handling.

Internally, the guest_auth_policy_id is simply a mapped-collection-only field matching the current structure, but the user_message treatment requires a new structure.

DocumentWithInducedDatatype now defines DATATYPE_VERSION_CALLBACKS as a secondary versioning mechanism alongside the
DATATYPE_VERSION_IMPLICATIONS mapping. The callbacks take the entire object and return an optional version number tuple. This is an extremely generic way of allowing a callback to apply arbitrary criteria for the version numbering. In service of that change, the protocol now requires __getitem__ (trivially satisfied by all instances) so that the contents can be checked.
The callbacks are defined as a classvar tuple.

Support versions '1.6.0' and '1.7.0' via `guest_auth_policy_id` and
long `user_message` handling.

Internally, the `guest_auth_policy_id` is simply a
mapped-collection-only field matching the current structure, but the
`user_message` treatment requires a new structure.

DocumentWithInducedDatatype now defines `DATATYPE_VERSION_CALLBACKS`
as a secondary versioning mechanism alongside the
`DATATYPE_VERSION_IMPLICATIONS` mapping. The callbacks take the entire
object and return an optional version number tuple. This is an
extremely generic way of allowing a callback to apply arbitrary
criteria for the version numbering. In service of that change, the
protocol now requires `__getitem__` (trivially satisfied by all instances)
so that the contents can be checked.
The callbacks are defined as a classvar tuple.

def __contains__(self, key: str) -> bool: # pragma: no cover
...

def __setitem__(self, key: str, value: t.Any) -> None: # pragma: no cover
...

def __getitem__(self, key: str) -> t.Any: # pragma: no cover
...

Copy link
Member Author

Choose a reason for hiding this comment

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

A brief note for reviewers:

I'm well aware that this protocol looks like it could inherit from Mapping. I've avoided making that change for two basic reasons:

  • I don't fully know what the implications are of protocol inheritance (normal class inheritance rules?)
  • Protocols are a way of getting non-inheritance based polymorphism. Using inheritance to define them strikes me instinctively as wrong, although maybe it's fine.

On both points, I'm mostly unsure and avoiding the issue for now.

Comment on lines -260 to +287
storage_gateway_id: t.Optional[UUIDLike] = None,
# strs
domain_name: t.Optional[str] = None,
guest_auth_policy_id: t.Optional[UUIDLike] = None,
storage_gateway_id: t.Optional[UUIDLike] = None,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like these type-based comment markers (# strs, # str lists) are becoming inaccurate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if they're useful or not, but I meant "is a string in the payload" (rather than the types accepted by the SDK helper). So the storage_gateway_id moved in part to try to make it accurate again.

Like I said, I'm not really sure if this is useful. I'm just trying to give us some handle for looking through these (long!) lists of arguments.

@sirosen sirosen merged commit f1f7ebd into globus:main Jan 31, 2023
@sirosen sirosen deleted the update-gcs-collection-versions branch January 31, 2023 20:51
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