Skip to content
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

Use @grafana/runtime instead of grafanaBootData #50

Merged
merged 6 commits into from
Jul 13, 2023
Merged

Conversation

idastambuk
Copy link
Contributor

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 idastambuk requested a review from a team as a code owner July 11, 2023 14:59
@idastambuk idastambuk requested review from fridgepoet and kevinwcyu and removed request for a team July 11, 2023 14:59
Copy link
Member

@fridgepoet fridgepoet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@idastambuk
Copy link
Contributor Author

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
Copy link
Contributor Author

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 grafana/grafana#71486

src/ConnectionConfig.tsx Outdated Show resolved Hide resolved
@idastambuk idastambuk merged commit cd94e70 into main Jul 13, 2023
3 checks passed
@idastambuk idastambuk deleted the refactor/runtime branch July 13, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants