search

grafana / grafana-aws-sdk-react

Apache License 2.0
3 stars 1 forks source link

Use @grafana/runtime instead of grafanaBootData #50

Closed idastambuk closed 1 year ago

idastambuk commented 1 year ago

This was started as a refactor of ConnectionConfig to remove (window as any), but even though there was a commit by @sunker that moved away from grafana/runtime to window.grafanaBootData (possibly because it was necessary for Grafana@7), I couldn't find a reason right now to not use config from runtime.

  1. One of the reasons /runtime.config seems a better solution is that here the grafanaBootData(config) is merged with defaults and with initial values from config.ts. This means that awsAssumeRoleEnabled and awsAllowedAuthProviders might not be defined in grafanaBootData, but instead in defaults for example, so it makes sense to read from a merged object (runtime.config) and not from grafanaBootData

  2. I also decided to remove test cases that check if these fields are defined since we have some code on the BE that takes care that both of these fields are defined and sets default values if they are not. So it looks to me that there is no way these fields could be undefined. This is also reflected in the types here:

    awsAllowedAuthProviders: string[] = [];
awsAssumeRoleEnabled = false;

    Lmk if there's anything I missed about the context here!

idastambuk commented 1 year ago

It would be great to have another review on this as I may have missed something.

For the assertions you've removed, so if I understand it's impossible to support that behavior in the frontend now? For the code you've referenced in the backend, are there tests protecting that behavior that we're depending on now?

  1. It would be possible to keep this in the FE, meaning we could still check if the fields aren't defined. However, when I tried to fix the types for these tests, I realized typescript was throwing an error saying I couldn't set the fields as undefined because they were defined as string[] or boolean here. I went digging in grafana a bit and saw that default values were being set here (BE) and here (FE). So I guess it didn't make sense that were checking for undefined in this repo, as these values can never be undefined. But that's up for discussion tbh: "Should code in this repo depend on values always being set in main grafana and that never changing?"
  2. Regarding tests in the BE, that's a good point. If we're removing tests from here, we should add them in the main grafana repo.
idastambuk commented 1 year ago

It would be great to have another review on this as I may have missed something. For the assertions you've removed, so if I understand it's impossible to support that behavior in the frontend now? For the code you've referenced in the backend, are there tests protecting that behavior that we're depending on now?

  1. It would be possible to keep this in the FE, meaning we could still check if the fields aren't defined. However, when I tried to fix the types for these tests, I realized typescript was throwing an error saying I couldn't set the fields as undefined because they were defined as string[] or boolean here. I went digging in grafana a bit and saw that default values were being set here (BE) and here (FE). So I guess it didn't make sense that were checking for undefined in this repo, as these values can never be undefined. But that's up for discussion tbh: "Should code in this repo depend on values always being set in main grafana and that never changing?"
  2. Regarding tests in the BE, that's a good point. If we're removing tests from here, we should add them in the main grafana repo.

PR for 2. here https://github.com/grafana/grafana/pull/71486