-
Notifications
You must be signed in to change notification settings - Fork 82
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
Improve image replication test region; Add LA notice #582
Conversation
regions, err := client.ListRegions(context.Background(), nil) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
regionHasCaps := func(r linodego.Region) bool { | ||
capsMap := make(map[string]bool) | ||
|
||
for _, c := range r.Capabilities { | ||
capsMap[strings.ToUpper(c)] = true | ||
} | ||
|
||
for _, c := range capabilities { | ||
if _, ok := capsMap[strings.ToUpper(c)]; !ok { | ||
return false | ||
} | ||
} | ||
|
||
return true | ||
} |
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.
optional: Would it make sense to call getRegionsWithCaps(...)
here and filter down the results below?
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.
I did try to call getRegionsWithCaps(...)
at first but figured out it might be better to have the logic here. Because getRegionsWithCaps(...)
only return a list of region's id string, we will have to make extra API calls to get the Region object for obtaining necessary info. It's probably slow down the test.
I'm wondering if it makes sense to move regionHasCaps(...)
as an independent function, so that we can call it here without repeating this part of logic. What's your thoughts?
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.
I think that would be a good idea. Maybe we can move it as a separate helper function in integration_test_suite.go
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.
@ykim-1 @lgarber-akamai thanks for the comment! Just applied the improvement, could both of you re-approve the PR?
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.
Image-related tests are all passing on my end, nice work!
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.
LGTM, tests passed locally with new fixture
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.
Looks great, nice work!
📝 Description
Improve the image replication test case and remove the hardcoded regions.
Add LA notice to image gen2 endpoint.
✔️ How to Test