-
Notifications
You must be signed in to change notification settings - Fork 801
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
Include Domain in GRPC UpdateDomainIsolationGroupsRequest #6114
Conversation
return &adminv1.UpdateDomainIsolationGroupsRequest{ | ||
Domain: t.Domain, |
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.
Could you change the title to reflect this bug fix?
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.
Done
@@ -1324,7 +1319,7 @@ func ToAdminUpdateDomainIsolationGroupsRequest(t *adminv1.UpdateDomainIsolationG | |||
} | |||
|
|||
func FromIsolationGroupConfig(in *types.IsolationGroupConfiguration) *apiv1.IsolationGroupConfiguration { | |||
if in == nil { | |||
if in == nil || *in == nil { |
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.
nit: Do we need to distinguish between initialized and uninitialized maps? If no, len(in)
might be more understandable.
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 way these tests are structured I'm trying to do an exact round trip.
Currently:
- in == nil -> nil
- *in == nil -> empty map
- len(*in) == 0 -> empty map
New behavior:
- in == nil -> nil
- *in == nil -> nil
- len(*in) == 0 -> empty map
For these specific mappers, since the parameter being passed in is a reference to the field on a request in
is never actually nil. It'll just be pointing to a nil value. There are other invocations of this method that could actually be passing in nil though.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018fea45-352e-4468-9978-57274eb4f684Details
💛 - Coveralls |
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes