OSD-4804 service-quota --all-regions#74
OSD-4804 service-quota --all-regions#74openshift-merge-robot merged 6 commits intoopenshift:masterfrom
Conversation
drewandersonnz
changed the title
drewandersonnz
requested review from
iamkirkbater and
rogbas
and removed request for
fahlmant
drewandersonnz
force-pushed
the
OSD4804-servicequota-all-regions
branch
from
3938fbc to
d87bfdb
Compare
drpaneas
left a comment
drpaneas
left a comment
There was a problem hiding this comment.
Great work @drewandersonnz :D Thank you very much for your time working on this and excellent work on writing tests for for common.go. Please see my comments on the review, here's a summary:
- I think we need to provide a better comment for the
GetSupportRegionsas it's not clear to me how it behaves unless I read your tests. - Some minor optional syntactical glitches
- Some function calls return values you don't check.
- There is no testing for
update.goanddescribe.go. We can consider putting that in another PR, so we don't block this one, since there was no testing before either for those files. I really have a bad feeling, because I think I saw functions no used and some dead code. Oh well ...
I'm open to discussion for any of the points :)
Remove excess variables and re-scope Update GetSupportedRegions comment for clarity
drewandersonnz
force-pushed
the
OSD4804-servicequota-all-regions
branch
from
c0365b7 to
b47f245
Compare
|
Thanks Drew I will check it soon |
drpaneas
left a comment
drpaneas
left a comment
There was a problem hiding this comment.
Is there any particular reason we need the commented code?
|
/lgtm |
| // GetSupportedRegions returns a []string of all supported regions if found and an error. Filtering for a single region is also supported. | ||
| func GetSupportedRegions(filter string, allRegions bool) ([]string, error) { | ||
| var results []string | ||
| for i := range awsv1alpha1.CoveredRegions { |
There was a problem hiding this comment.
Nice, I like how it's consuming from the main AWS account operator code.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: drewandersonnz, drpaneas, rogbas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
…ota-all-regions OSD-4804 service-quota --all-regions
5 participants
Allow use of --all-regions to loop through supported regions and apply servicequota commands in sequence