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 flows.update_flow(...) #710

Merged
merged 2 commits into from
Mar 24, 2023

Conversation

derek-globus
Copy link
Contributor

@derek-globus derek-globus commented Mar 17, 2023

sc-22685
Added the update_flow method to FlowsClient

Direct Generated Docs Link: FlowsClient.update_flow()

Related Changes

I additionally modified the create_flow docs to be slightly shorter (by using sphinx-design dropdowns) and pulled the example usage into a tabbed view along with an example response

  • This change felt small & related enough to update_flow but I can rip it out of this PR if anyone would prefer I submit it separately

Testing

  • Hit flows with various normal editing permutations of flow definition, input_schema, flow_viewers, etc.
  • Tested the flow_owner update (this one felt fairly special-cased so I wanted to verify it specifically)
    • Created a flow with user A with user B as flow_administrator
    • As user B, called `flows.update_flow(flow_id, flow_owner=user_b_urn)
    • Observed user B now owned the Flow

📚 Documentation preview 📚: https://globus-sdk-python--710.org.readthedocs.build/en/710/

@derek-globus derek-globus changed the title Added flows.update-flow(...) Added flows.update_flow(...) Mar 17, 2023
src/globus_sdk/services/flows/client.py Outdated Show resolved Hide resolved
display.
:type description: str (0 - 4096 characters), optional
:param flow_owner: An Auth Identity URN to set as flow owner; this must match
the Identity URN of the entity calling `update_flow`
Copy link
Member

Choose a reason for hiding this comment

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

"the" identity URN, or "an" identity URN of any of the caller's linked accounts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hadn't considered this.
Lemme verify that it is in fact any identity associated with the new-owner account.

Good callout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some testing, "the Identity URN" is correct.

Linked accounts are not allowed to set each other as the owner of a flow.


Given the following:
3 identities:

  • A-1
  • A-2
  • B

where A-1 & A-2 are linked identities

I did the following:

  1. As B: Create a flow (B is implicitly owner)
  2. As B: Add A-1, A-2, B as Administrators of the flow
  3. As A-1, attempt to set A-2 as FlowOwner -> Error 400
    •  "Invalid value for field 'owner': if present it must match the identity of the calling user (<A-1's URN>) and that user must be an Administrator of the Flow"
      
  4. As A-1, attempt to set A-1 as Flow Owner -> success
  5. As A-1, attempt again to set A-2 as Flow Owner -> Same 400 as in step (3)

Copy link
Member

Choose a reason for hiding this comment

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

I want to make a case for changing this behavior in the service. I'll be bringing this up later today.
As such, I think the current phrasing is good but also that we shouldn't necessarily worry about subtle behaviors at play here.

src/globus_sdk/services/flows/client.py Outdated Show resolved Hide resolved
src/globus_sdk/services/flows/client.py Show resolved Hide resolved
src/globus_sdk/services/flows/client.py Outdated Show resolved Hide resolved
src/globus_sdk/services/flows/client.py Outdated Show resolved Hide resolved
flow_administrators: list[str] | None = None,
keywords: list[str] | None = None,
additional_fields: dict[str, t.Any] | None = None,
) -> GlobusHTTPResponse:
Copy link
Member

@sirosen sirosen Mar 20, 2023

Choose a reason for hiding this comment

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

We hit a snag which we were discussing regarding the fact that this signature and the nearby signature for create_flow don't't follow the same pattern used by other clients within the SDK in which a dedicated object serves as a payload builder, and the client method submits that payload.
(Examples: see TransferClient.submit_transfer, SearchClient.scroll, and GCSClient.create_collection)

One thing to note about this is that we can't necessarily implement the interface here -- a method which builds the payload and submits it -- with confidence for all of the APIs we need to support. Several APIs have incomplete docs, not everything is in OpenAPI, and we are sometimes "flying blind" to some degree. I think we can more reliably implement the dict and payload helper interface than we can do methods like update_flow. (It also evolves well as we start with a method accepting a dict and only later add support for a payload helper.)

I've been continuing to think about this and have an opinion with a proposed plan of action:
Consistency within the client class matters more than consistency across the SDK in the short term. I'd like the FlowsClient to be consistent with the other clients, but that will require deciding how we want to resolve that inconsistency for create_flow. I do not see any advantage in making update_flow different and then having to transition create_flow to match it. We should introduce this in the same manner as create_flow and then if desired change them both.

❗ Therefore, we acknowledge the divergence between the client classes and consider it a future problem.

There are other methods in the SDK which "don't belong", like TransferClient.task_wait. Our landscape of client methods is variegated -- one might even call it motley -- but this is a livable situation as long as we make the docs good. I'd like to think about how to resolve these inconsistencies, but it's not necessarily a "today problem".

Some example ideas to bat around:

  • always only have the payload / helper -based method (as I mentioned above, I don't think we can always implement the keyword-argument formulation)
  • have both a simple API with no payload helpers, like this, and some "conventionally named" underlying method of submitting a dict or helper document (examples: submit_update_flow / update_flow, send_flow_update / update_flow)
  • allow client objects to have an attached facade as a related object; e.g. FlowsClient.update_flow takes a payload, FlowsManager.update_flow takes various args and calls FlowsClient.update_flow, and FlowsClient(...).manager is a FlowsManager

display.
:type description: str (0 - 4096 characters), optional
:param flow_owner: An Auth Identity URN to set as flow owner; this must match
the Identity URN of the entity calling `update_flow`
Copy link
Member

Choose a reason for hiding this comment

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

I want to make a case for changing this behavior in the service. I'll be bringing this up later today.
As such, I think the current phrasing is good but also that we shouldn't necessarily worry about subtle behaviors at play here.

@derek-globus derek-globus merged commit ce5975f into globus:main Mar 24, 2023
@derek-globus derek-globus deleted the implement-update-flow-sc-22685 branch March 24, 2023 18:06
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