Regions Fixes #136
Regions Fixes #136
Conversation
|
Backend code coverage report for PR #136
|
|
Frontend code coverage report for PR #136
|
- remove unused files - fix typos
There was a problem hiding this comment.
Looks good! I think it would have been could if the two issues were in two different PRs if we ever needed to revert one of the two. Also makes it easier for users to understand the changelog.
Now in the ConfigEditor, the standard regions from grafana-aws-sdk-react is being used. This is not the end of the world, but it would be better to use the same list of region here as in the query editor. I think we have a few options here:
- Spend some more time investigating if regionsForService api is available in the javascript sdk. If so, the ConnectionConfig could take "service" as a prop and then resolve regions for the service inside the component. This is nice because it would work for all our plugins, and it would also work "the first time" the config editor is loaded since it doesn't require a backend (the plugin to be saved).
- Load regions for service from the backend. This will require some work because the first time the config editor is loaded, there's no backend because it's not saved. However, it has been solved in for example CloudWatch so should be possible to do here too.
- Keep a hard coded, but updated, list of regions in the ConfigEditor (and pass it to ConnectionConfig).
If there's a javascript api for regionsForService, that's the best option because it would be the least amount of work for all plugins that are consuming the grafana-aws-sdk-react. But if we want to, we can go ahead and merge this PR with a hard coded (but updated) list of regions.
Thoughts?
| @@ -360,6 +378,15 @@ func TestDatasource(t *testing.T) { | |||
| require.Equal(t, "graf2 (AWS2)", response.Responses["A"].Frames[0].Fields[6].At(0)) | |||
| }) | |||
|
|
|||
| t.Run("getInsightSummaries query with different region", func(t *testing.T) { | |||
There was a problem hiding this comment.
What does "with different region" mean in this context? Different than the default region?
There was a problem hiding this comment.
yeah different from the default
|
Yeah I'll break this up into 2 prs :) Regarding updates to the config page, yeah that's interesting. I think if I recall correctly regionsForService didn't exist in the javascript sdk, but maybe there is another call with a different name that we can use. If I don't find anything I'll start looking into making a request to our backend for it. |
Fixes #138
and
Fixes #137
Shoutout to "the mob" for joining in on this one, added some tests and found another bug to fix.