-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
chore(hybridcloud) Expand OrgMapping update payload #71119
Conversation
Our schema generation and validation tooling doesn't work well with tuple values. Replace this tuple with an 'Optional' that retains the ability to preserve 'undefined' vs 'defined and none'. Refs HC-1190
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #71119 +/- ##
==========================================
+ Coverage 77.89% 77.91% +0.02%
==========================================
Files 6529 6526 -3
Lines 290909 290386 -523
Branches 50338 50229 -109
==========================================
- Hits 226591 226254 -337
+ Misses 58096 57896 -200
- Partials 6222 6236 +14
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, just had a minor question about the param types
@@ -32,7 +36,8 @@ class RpcOrganizationMappingUpdate(RpcModel): | |||
# When not set, no change to customer id performed, | |||
# when set with a tuple, the customer_id set to either None or the string | |||
# that is the first element. | |||
customer_id: tuple[str | None] | None = None | |||
# Using a tuple is deprecated and will be removed. | |||
customer_id: tuple[str | None] | CustomerId | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of using a class here vs just passing an int? Will we be adding more attributes to it in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation uses a single element tuple of str | None
to indicate a value/explicit None vs an implicit None because the key is undefined. The class lets us wrap None and maintain the same semantics as the tuple.
Our schema generation and validation tooling doesn't work well with tuple values. Replace this tuple with an 'Optional' that retains the ability to preserve 'undefined' vs 'defined and none'.
Refs HC-1190