Repository navigation
feat: added data residency for eu and global regions - #469
Conversation
| func setDataResidency(options options) (string, error) { | ||
| currentHost := options.Host | ||
| defaultHost := allowedRegionsHostMap["global"] | ||
| if currentHost != defaultHost { // for testing, the hostname can be different |
There was a problem hiding this comment.
What do you mean hostname can be different? For example if its api.sendgrid.com by default and I am calling setDataResidency('eu'), shouldn't the hostname change ?
There was a problem hiding this comment.
In the test cases, we set the URL of the fakeServer as the hostname (something like 127.0.0.1:5000) In that case if we return the default host (api.sendgrid.com) the test would fail. That's why I added this check.
| // SetDataResidency : Set region for sendgrid. | ||
| func SetDataResidencyGlobal() { | ||
| message := buildHelloEmail() | ||
| request := buildSendgridObj("global") |
There was a problem hiding this comment.
Can we call setDataResidency() method on the request object too? Right now, I guess the only way to set it is via the constructor, no?
|
LGTM |
| } | ||
| } | ||
|
|
||
| func ExtractEndpoint(link string) (string, error) { |
There was a problem hiding this comment.
is there any reason to export this outside of the sendgrid package? If not, we should make it private
There was a problem hiding this comment.
caling this exportPath seems more appropriate too
There was a problem hiding this comment.
yeah we do not need to export it outside, making it private.
|
|
||
| func SetDataResidency(request rest.Request, region string) (rest.Request, error) { | ||
| regionalHost, isPresent := allowedRegionsHostMap[region] | ||
| if isPresent { |
There was a problem hiding this comment.
it's generally better to wrap the !ok in the if block since it's a shorter path. You'll be able to avoid the nested if blocks and makes the code more readable. So something like
regionalHost, ok := allowedRegionsHostMap[region]
if !ok {
return error
}
do other stuff here
There was a problem hiding this comment.
Yeah that makes sense. Will correct it
| ShouldHaveHeaders(&request, t) | ||
| } | ||
|
|
||
| func Test_test_set_residency_eu(t *testing.T) { |
There was a problem hiding this comment.
we should try to follow the Go convention of naming test functions and proper casing. You can refer to the other existing tests for example, but generally, it should be Test<FunctionNameBeingTested>
There was a problem hiding this comment.
Right, will correct the casing and naming of the test cases
There was a problem hiding this comment.
I think for different scenarios in a single function to be tested, we can add them with an underscore like the test cases like TestRequestRetry_rateLimit and TestRequestRetry_rateLimit_noHeader.
Similarly, I think we can also have test cases named - TestSetDataResidency_eu and TestSetDataResidency_global. Will that be okay?
| return request, nil | ||
| } | ||
|
|
||
| func SetDataResidency(request rest.Request, region string) (rest.Request, error) { |
There was a problem hiding this comment.
we should add comments to document what these function does, especially how it manipulates the host field
| return parsedURL.Path, nil | ||
| } | ||
|
|
||
| func SetHost(request rest.Request, host string) (rest.Request, error) { |
There was a problem hiding this comment.
public functions should be documented
* feat: added data residency for eu and global regions * feat: added data residency for eu and global regions * fix: added setters for host and data residency * chore: corrected the naming of test cases * chore: added inline documentation for SetDataResidency and SetHost * chore: added comment for SetDataResidency
Fixes
Added region field in sendgrid options. The default region is "global" and the default host is "api.sendgrid.com". When the region is set to "eu", the host should change to "api.eu.sendgrid.com".
Checklist
If you have questions, please file a support ticket.