GCS/GCP Subscription ID Management #921
Conversation
derek-globus
requested review from
aaschaer,
ada-globus,
kurtmckee and
sirosen
as code owners
changelog.d/20231215_140223_derek_gcs_subscription_id_sc_28587.md
Outdated
Show resolved
Hide resolved
changelog.d/20231215_140223_derek_gcs_subscription_id_sc_28587.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Kurt McKee <[email protected]>
Co-authored-by: Kurt McKee <[email protected]>
| "set-subscription-id": ( | ||
| "endpoint.set_subscription_id", | ||
| "set_endpoint_subscription_id", | ||
| ), |
There was a problem hiding this comment.
I'm okay with this as a short-term position, but I'd like us to write a new command for this, rather than reusing the existing one.
My reason is primarily helptext and parameter names. If you look at the existing command, you'll see it states
Unlike the '--managed' flag for 'globus endpoint update' ...
Which is not something we want to retain into the future.
I think it's also worth having the new command check to confirm that it's operating on a GCP_mapped_collection rather than a GCP_guest_collection. That way, we more clearly explain to users that it's only possible to use the relevant command on a GCP host, not the shares underneath that host.
I'm perfectly happy for us to pursue that change as a fast-follow to this PR though.
There was a problem hiding this comment.
I initially did write a new command for GCP but erased it when it ended up being identical to this one.
If we do add a duplicate code path, it's worth noting that introduces the risk of drift (one path gets updated but not the other).
If it's just a helptext concern, I don't think that's worth a copy/paste. If it's adding additional validation of mapped vs guest collection types; I could buy it. It's a fairly small copy/paste.
I'll include that in a new rev of this PR.
There was a problem hiding this comment.
I'm not that concerned about the paths being duplicated, since I think they are actually different. The question IMO is only how much it's worth working hard to avoid code duplication.
I want the following:
- GCP-specific checks in the GCP command
- legacy-specific deprecations in the legacy command
- different helptext for the two
The worry about drift would be more pronounced if this didn't take the form of a deprecation. However, I don't think we want to keep globus endpoint (the whole tree), so we should be making moves to deprecate its various pieces.
|
|
||
| def convert( | ||
| self, value: str, param: click.Parameter | None, ctx: click.Context | None | ||
| ) -> t.Any: |
There was a problem hiding this comment.
It's not super-closely related to this PR, but just because I think it's cool, I wanted to mention that under the latest click-type-test, you can write convert(...) -> uuid.UUID | ExplicitNullType and omit get_type_annotation.
There was a problem hiding this comment.
With #923 merged, you could now apply this change here (after a rebase).
Co-authored-by: Stephen Rosen <[email protected]>
sc-28587
What?
globus gcs endpoint set-subscription-idcommand which allows subscription managers and endpoint admins to modify the subscription ID for a gcs endpoint.globus gcp set-subscription-idwhich aliases existingglobus endpoint set-subscription-idcommand, allowing subscription managers & collection admins to modify the subscription ID for a gcp collection.Testing
Stood up a gcs endpoint in sandbox, tested setting/removing the subscription for:
Stood up a gcp endpoint in sandbox, tested setting/removing the subscription for: