Update SDK to set default project when GCE auth is chosen #7
Conversation
- Update authTypeChange function to retrieve project using backendSrv - Improved error handling - Update type - Update CHANGELOG - Include @grafana/runtime package - Update tests
There was a problem hiding this comment.
thanks for the PR! this project setting is a bit messy 🤔 Ideally I would just make it transparent and not differentiate between gceDefaultProject and defaultProject but that would be a bigger change. In any case, I think that this is better solved in the plugin code rather than here. See my last comment for my reasoning, let me know what you think
| @@ -23,6 +23,7 @@ | |||
| "dependencies": {}, | |||
| "devDependencies": { | |||
| "@grafana/data": "^8.2.1", | |||
| "@grafana/runtime": "8.4.0-pre", | |||
There was a problem hiding this comment.
Is there any reason to pin to this pre-release? I would use a version range like the other @grafana/* deps
| import { TEST_IDS } from 'testIds'; | ||
| import { JWTConfigEditor } from './components/JWTConfigEditor'; | ||
| import { JWTForm } from './components/JWTForm'; | ||
| import { GOOGLE_AUTH_TYPE_OPTIONS } from './constants'; | ||
| import { DataSourceOptions, DataSourceSecureJsonData, GoogleAuthType } from './types'; | ||
|
|
||
| export type ConfigEditorProps = DataSourcePluginOptionsEditorProps<DataSourceOptions, DataSourceSecureJsonData>; | ||
| export type ConfigEditorProps = DataSourcePluginOptionsEditorProps<DataSourceOptions, DataSourceSecureJsonData> & { | ||
| backendSrv?: BackendSrv; |
There was a problem hiding this comment.
I think that rather than passing this from the parent, I would just instantiate the backendSrv in this component. That way, the way to instantiate the component won't change (and it's as easy as today, avoiding breaking changes).
If you want to leave it as it is, I would at least make it mandatory so people don't miss the fact that the backendSrv is necessary for the gce case.
| try { | ||
| if (authenticationType === 'gce' && !gceDefaultProject) { | ||
| gceDefaultProject = await props.backendSrv.get( | ||
| `/api/datasources/${props.options.id}/resources/gceDefaultProject` | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| setError(err.data.message); | ||
| } |
There was a problem hiding this comment.
actually, I don't know if it's the best to do this here. It's doing a backend call before the settings are saved. This means that people will get an error just by clicking the auth type switch (if they are not in GCE) and any other setting needed for the configuration won't be taken into account.
I think that the problem is that the cloud-monitoring plugin is defining this gceDefaultProject as part of the JsonData, but it's not (since it's not being saved here). I would remove it from there:
And replace it with Datasource private property, initializing its value in the constructor. That way we avoid this ambiguity.
There was a problem hiding this comment.
I don't think it's necessarily a problem that we're doing a backend call here before settings are saved but I see your point on the error.
I agree on the comment around the two different project names and I'll look into potentially making that change instead. Setting the project name in the datasource would also avoid any issues if an instance were to be moved to a different project.
|
Closing in favour of grafana/grafana#53652 |
Related to grafana/grafana#51327