feat(asset): add variable query #71
Conversation
TigranVardanyan
requested review from
CiprianAnton and
kkerezsi
as code owners
|
Have we started the review yet? @CiprianAnton @kkerezsi |
|
|
||
| export type Props = QueryEditorProps<AssetDataSource, AssetQuery> | ||
|
|
||
| export class AssetQueryEditorCommon { |
There was a problem hiding this comment.
This is kind of a strange implementation here. you are basically having a common class that you use in 2 places. You are also creating references to your methods from the datasource and components . I think a better solution here is to create a custom react hook and pass the methods in the components here needed.
|
|
||
| readonly loadMinionIdOptions = (ids: SystemMetadata[] | void): Array<SelectableValue<string>> => { | ||
| if (!ids) { | ||
| return [] |
There was a problem hiding this comment.
you are missing the endline ;
|
|
||
| const onWorkspaceChange = (item?: SelectableValue<string>): void => { | ||
| if (item?.value && item.value !== query.workspace) { | ||
| // if workspace changed, reset Systems and Assets fields |
There was a problem hiding this comment.
Let's just remove all comments from this component
| import { AssetQuery } from "../types"; | ||
| import { FloatingError, parseErrorMessage } from "../../../core/errors"; | ||
|
|
||
| export function AssetVariableQueryEditor(props: Props) { |
There was a problem hiding this comment.
Add a description/summary to your component instead. Something like :
This component is going to be used to render Assets as part of Grafana Variables.
The following components will query for assets based by workspace and system
| const render = setupRenderer(AssetVariableQueryEditor, FakeAssetDataSource); | ||
| const workspacesLoaded = () => waitForElementToBeRemoved(screen.getByTestId('Spinner')); | ||
|
|
||
|
|
| expect(screen.getByText('1')).toBeInTheDocument(); | ||
|
|
||
| // User selects different workspace | ||
| await select(screen.getAllByRole('combobox')[0], 'Default workspace', { container: document.body }); |
There was a problem hiding this comment.
We can test what happens when the call to the backend fails.
We should get a floating error right ?
| conditions.push(`workspace = "${workspace}"`); | ||
| } | ||
| const assetFilter = conditions.join(' and '); | ||
| const assetsResponse: AssetModel[] = await this.queryAssets(assetFilter, 1000); |
There was a problem hiding this comment.
I think this we need to specify in the HLD and discuss with Josh. Should we query for 1000 assets here ? We also don't have projection here so it will eat up a lot of unnecessary resources both on client and on the backend
| @@ -98,6 +99,22 @@ export class AssetDataSource extends DataSourceBase<AssetMetadataQuery> { | |||
| } | |||
| } | |||
|
|
|||
| async metricFindQuery({ minionIds, workspace }: AssetQuery): Promise<MetricFindValue[]> { | |||
| const conditions = []; | |||
| if (minionIds?.length) { | |||
There was a problem hiding this comment.
Here we'd probably want to implement a query builder using the builder pattern to reduce complexity
|
@TigranVardanyan I think here we'd need to get @joshuaprewitt feedback here to make sure that we are not building something that will eventually be rewritten to due missing/wrong requirements. Please add a more detailed explanation and provide some screenshots to highlight how this code change will affect the product in the PR description. |
Pull Request
🤨 Rationale
Added variable query to Asset datasource
👩💻 Implementation
🧪 Testing
✅ Checklist