From 590d1f8a7dbccada0f66dc1ad987f528f7aa2990 Mon Sep 17 00:00:00 2001 From: Andreas Christou Date: Mon, 23 Jan 2023 12:19:48 +0000 Subject: [PATCH] AzureMonitor: Move DefaultSubscriptions field to separate component (#61633) * Move DefaultSubscriptions field to separate component - Add separate component - Update CredentialsForm - Update tests - Remove redundant test * Remove defaultSubscriptionId - Update MonitorConfig to appropriately set subscriptionId - Update DefaultSubscription component to make use of JsonData - Remove references to defaultSubscriptionId - Update tests - Update mockInstanceSettings for flexibility * Review - Add DefaultSubscription test and remove component from AzureCredentialsForm test - Remove onCredentialsChange from DefaultSubscription - Set subscription as a part of onSubscriptionChange --- .../__mocks__/instanceSettings.ts | 11 +- .../components/AzureCredentialsForm.test.tsx | 38 ------- .../components/AzureCredentialsForm.tsx | 99 +---------------- .../components/DefaultSubscription.test.tsx | 58 ++++++++++ .../components/DefaultSubscription.tsx | 103 ++++++++++++++++++ .../components/MonitorConfig.tsx | 34 +++++- .../credentials.ts | 4 - .../types/types.ts | 1 - 8 files changed, 200 insertions(+), 148 deletions(-) create mode 100644 public/app/plugins/datasource/grafana-azure-monitor-datasource/components/DefaultSubscription.test.tsx create mode 100644 public/app/plugins/datasource/grafana-azure-monitor-datasource/components/DefaultSubscription.tsx diff --git a/public/app/plugins/datasource/grafana-azure-monitor-datasource/__mocks__/instanceSettings.ts b/public/app/plugins/datasource/grafana-azure-monitor-datasource/__mocks__/instanceSettings.ts index d857b8c0bc5..e50f92359d0 100644 --- a/public/app/plugins/datasource/grafana-azure-monitor-datasource/__mocks__/instanceSettings.ts +++ b/public/app/plugins/datasource/grafana-azure-monitor-datasource/__mocks__/instanceSettings.ts @@ -1,8 +1,11 @@ -import { DataSourcePluginMeta } from '@grafana/data'; +import { DataSourceInstanceSettings, DataSourcePluginMeta } from '@grafana/data'; -import { AzureDataSourceInstanceSettings } from '../types'; +import { AzureDataSourceInstanceSettings, AzureDataSourceJsonData } from '../types'; -export const createMockInstanceSetttings = (): AzureDataSourceInstanceSettings => ({ +export const createMockInstanceSetttings = ( + overrides?: Partial, + jsonDataOverrides?: Partial +): AzureDataSourceInstanceSettings => ({ url: '/ds/1', id: 1, uid: 'abc', @@ -11,6 +14,7 @@ export const createMockInstanceSetttings = (): AzureDataSourceInstanceSettings = meta: {} as DataSourcePluginMeta, name: 'azure', readOnly: false, + ...overrides, jsonData: { cloudName: 'azuremonitor', @@ -20,5 +24,6 @@ export const createMockInstanceSetttings = (): AzureDataSourceInstanceSettings = tenantId: 'abc-123', clientId: 'def-456', subscriptionId: 'ghi-789', + ...jsonDataOverrides, }, }); diff --git a/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.test.tsx b/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.test.tsx index 367af9ebccb..a14308429e9 100644 --- a/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.test.tsx +++ b/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.test.tsx @@ -1,8 +1,6 @@ import { render, screen, waitFor } from '@testing-library/react'; import React from 'react'; -import { selectors } from '../e2e/selectors'; - import AzureCredentialsForm, { Props } from './AzureCredentialsForm'; const setup = (propsFunc?: (props: Props) => Props) => { @@ -14,7 +12,6 @@ const setup = (propsFunc?: (props: Props) => Props) => { tenantId: 'e7f3f661-a933-3h3f-0294-31c4f962ec48', clientId: '34509fad-c0r9-45df-9e25-f1ee34af6900', clientSecret: undefined, - defaultSubscriptionId: '44987801-6nn6-49he-9b2d-9106972f9789', }, azureCloudOptions: [ { value: 'azuremonitor', label: 'Azure' }, @@ -22,7 +19,6 @@ const setup = (propsFunc?: (props: Props) => Props) => { { value: 'chinaazuremonitor', label: 'Azure China' }, ], onCredentialsChange: jest.fn(), - getSubscriptions: jest.fn(() => Promise.resolve([])), }; if (propsFunc) { @@ -54,22 +50,6 @@ describe('Render', () => { expect(screen.getByTestId('client-secret')).toBeDisabled(); }); - it('should enable azure monitor load subscriptions button', async () => { - setup((props) => ({ - ...props, - credentials: { - authType: 'clientsecret', - azureCloud: 'azuremonitor', - tenantId: 'e7f3f661-a933-3h3f-0294-31c4f962ec48', - clientId: '34509fad-c0r9-45df-9e25-f1ee34af6900', - clientSecret: 'e7f3f661-a933-4b3f-8176-51c4f982ec48', - }, - })); - await waitFor(() => - expect(screen.getByTestId(selectors.components.configEditor.loadSubscriptions.button)).not.toBeDisabled() - ); - }); - describe('when disabled', () => { it('should disable inputs', async () => { setup((props) => ({ @@ -80,23 +60,5 @@ describe('Render', () => { await waitFor(() => screen.getByLabelText('Azure Cloud')); expect(screen.getByLabelText('Azure Cloud')).toBeDisabled(); }); - - it('should remove buttons', async () => { - setup((props) => ({ - ...props, - disabled: true, - })); - await waitFor(() => - expect(screen.getByTestId(selectors.components.configEditor.loadSubscriptions.button)).toBeDisabled() - ); - }); - - it('should render children components', () => { - setup((props) => ({ - ...props, - children: , - })); - expect(screen.getByText('click me')).toBeInTheDocument(); - }); }); }); diff --git a/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.tsx b/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.tsx index 33eb02706ef..48b652fec9e 100644 --- a/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.tsx +++ b/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.tsx @@ -1,9 +1,8 @@ -import React, { ChangeEvent, FunctionComponent, useEffect, useReducer, useState } from 'react'; +import React, { ChangeEvent, FunctionComponent } from 'react'; import { SelectableValue } from '@grafana/data'; import { LegacyForms, Button, Select, InlineField } from '@grafana/ui'; -import { isCredentialsComplete } from '../credentials'; import { selectors } from '../e2e/selectors'; import { AzureAuthType, AzureCredentials } from '../types'; @@ -14,7 +13,6 @@ export interface Props { credentials: AzureCredentials; azureCloudOptions?: SelectableValue[]; onCredentialsChange?: (updatedCredentials: AzureCredentials) => void; - getSubscriptions?: () => Promise; disabled?: boolean; children?: JSX.Element; } @@ -33,52 +31,13 @@ const authTypeOptions: Array> = [ const LABEL_WIDTH = 18; export const AzureCredentialsForm: FunctionComponent = (props: Props) => { - const { credentials, azureCloudOptions, onCredentialsChange, getSubscriptions, disabled } = props; - const hasRequiredFields = isCredentialsComplete(credentials); - - const [subscriptions, setSubscriptions] = useState>>([]); - const [loadSubscriptionsClicked, onLoadSubscriptions] = useReducer((val) => val + 1, 0); - useEffect(() => { - if (!getSubscriptions || !hasRequiredFields) { - updateSubscriptions([]); - return; - } - let canceled = false; - getSubscriptions().then((result) => { - if (!canceled) { - updateSubscriptions(result, loadSubscriptionsClicked); - } - }); - return () => { - canceled = true; - }; - // This effect is intended to be called only once initially and on Load Subscriptions click - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [loadSubscriptionsClicked]); - - const updateSubscriptions = (received: Array>, autoSelect = false) => { - setSubscriptions(received); - if (getSubscriptions) { - if (autoSelect && !credentials.defaultSubscriptionId && received.length > 0) { - // Selecting the default subscription if subscriptions received but no default subscription selected - onSubscriptionChange(received[0]); - } else if (credentials.defaultSubscriptionId) { - const found = received.find((opt) => opt.value === credentials.defaultSubscriptionId); - if (!found) { - // Unselecting the default subscription if it isn't found among the received subscriptions - onSubscriptionChange(undefined); - } - } - } - }; + const { credentials, azureCloudOptions, onCredentialsChange, disabled, managedIdentityEnabled } = props; const onAuthTypeChange = (selected: SelectableValue) => { if (onCredentialsChange) { - setSubscriptions([]); const updated: AzureCredentials = { ...credentials, authType: selected.value || 'msi', - defaultSubscriptionId: undefined, }; onCredentialsChange(updated); } @@ -86,11 +45,9 @@ export const AzureCredentialsForm: FunctionComponent = (props: Props) => const onAzureCloudChange = (selected: SelectableValue) => { if (onCredentialsChange && credentials.authType === 'clientsecret') { - setSubscriptions([]); const updated: AzureCredentials = { ...credentials, azureCloud: selected.value, - defaultSubscriptionId: undefined, }; onCredentialsChange(updated); } @@ -98,11 +55,9 @@ export const AzureCredentialsForm: FunctionComponent = (props: Props) => const onTenantIdChange = (event: ChangeEvent) => { if (onCredentialsChange && credentials.authType === 'clientsecret') { - setSubscriptions([]); const updated: AzureCredentials = { ...credentials, tenantId: event.target.value, - defaultSubscriptionId: undefined, }; onCredentialsChange(updated); } @@ -110,11 +65,9 @@ export const AzureCredentialsForm: FunctionComponent = (props: Props) => const onClientIdChange = (event: ChangeEvent) => { if (onCredentialsChange && credentials.authType === 'clientsecret') { - setSubscriptions([]); const updated: AzureCredentials = { ...credentials, clientId: event.target.value, - defaultSubscriptionId: undefined, }; onCredentialsChange(updated); } @@ -122,11 +75,9 @@ export const AzureCredentialsForm: FunctionComponent = (props: Props) => const onClientSecretChange = (event: ChangeEvent) => { if (onCredentialsChange && credentials.authType === 'clientsecret') { - setSubscriptions([]); const updated: AzureCredentials = { ...credentials, clientSecret: event.target.value, - defaultSubscriptionId: undefined, }; onCredentialsChange(updated); } @@ -134,21 +85,9 @@ export const AzureCredentialsForm: FunctionComponent = (props: Props) => const onClientSecretReset = () => { if (onCredentialsChange && credentials.authType === 'clientsecret') { - setSubscriptions([]); const updated: AzureCredentials = { ...credentials, clientSecret: '', - defaultSubscriptionId: undefined, - }; - onCredentialsChange(updated); - } - }; - - const onSubscriptionChange = (selected: SelectableValue | undefined) => { - if (onCredentialsChange) { - const updated: AzureCredentials = { - ...credentials, - defaultSubscriptionId: selected?.value, }; onCredentialsChange(updated); } @@ -156,7 +95,7 @@ export const AzureCredentialsForm: FunctionComponent = (props: Props) => return (
- {props.managedIdentityEnabled && ( + {managedIdentityEnabled && ( = (props: Props) => ))} )} - {getSubscriptions && ( - -
- opt.value === options.subscriptionId) : undefined + } + options={subscriptions} + onChange={onChange} + disabled={disabled} + /> + +
+ + + ); +}; diff --git a/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/MonitorConfig.tsx b/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/MonitorConfig.tsx index eeba7a88183..0ad6e73a59c 100644 --- a/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/MonitorConfig.tsx +++ b/public/app/plugins/datasource/grafana-azure-monitor-datasource/components/MonitorConfig.tsx @@ -1,4 +1,4 @@ -import React, { FunctionComponent, useMemo } from 'react'; +import React, { FunctionComponent, useMemo, useState } from 'react'; import { SelectableValue } from '@grafana/data'; import { config } from '@grafana/runtime'; @@ -7,6 +7,7 @@ import { getCredentials, updateCredentials } from '../credentials'; import { AzureDataSourceSettings, AzureCredentials } from '../types'; import { AzureCredentialsForm } from './AzureCredentialsForm'; +import { DefaultSubscription } from './DefaultSubscription'; const azureClouds = [ { value: 'azuremonitor', label: 'Azure' }, @@ -21,13 +22,25 @@ export interface Props { } export const MonitorConfig: FunctionComponent = (props: Props) => { - const { updateOptions, getSubscriptions } = props; + const { updateOptions, getSubscriptions, options } = props; + const [subscriptions, setSubscriptions] = useState>>([]); const credentials = useMemo(() => getCredentials(props.options), [props.options]); - const onCredentialsChange = (credentials: AzureCredentials): void => { - updateOptions((options) => updateCredentials(options, credentials)); + const onCredentialsChange = (credentials: AzureCredentials, subscriptionId?: string): void => { + if (!subscriptionId) { + setSubscriptions([]); + } + updateOptions((options) => + updateCredentials({ ...options, jsonData: { ...options.jsonData, subscriptionId } }, credentials) + ); }; + const onSubscriptionsChange = (receivedSubscriptions: Array>) => + setSubscriptions(receivedSubscriptions); + + const onSubscriptionChange = (subscriptionId?: string) => + updateOptions((options) => ({ ...options, jsonData: { ...options.jsonData, subscriptionId } })); + return ( <>

Authentication

@@ -36,9 +49,18 @@ export const MonitorConfig: FunctionComponent = (props: Props) => { credentials={credentials} azureCloudOptions={azureClouds} onCredentialsChange={onCredentialsChange} - getSubscriptions={getSubscriptions} disabled={props.options.readOnly} - /> + > + + ); }; diff --git a/public/app/plugins/datasource/grafana-azure-monitor-datasource/credentials.ts b/public/app/plugins/datasource/grafana-azure-monitor-datasource/credentials.ts index bd5fb9139b0..d632feffc89 100644 --- a/public/app/plugins/datasource/grafana-azure-monitor-datasource/credentials.ts +++ b/public/app/plugins/datasource/grafana-azure-monitor-datasource/credentials.ts @@ -92,7 +92,6 @@ export function getCredentials(options: AzureDataSourceSettings): AzureCredentia if (config.azure.managedIdentityEnabled) { return { authType: 'msi', - defaultSubscriptionId: options.jsonData.subscriptionId, }; } else { // If authentication type is managed identity but managed identities were disabled in Grafana config, @@ -109,7 +108,6 @@ export function getCredentials(options: AzureDataSourceSettings): AzureCredentia tenantId: options.jsonData.tenantId, clientId: options.jsonData.clientId, clientSecret: getSecret(options), - defaultSubscriptionId: options.jsonData.subscriptionId, }; } } @@ -129,7 +127,6 @@ export function updateCredentials( jsonData: { ...options.jsonData, azureAuthType: 'msi', - subscriptionId: credentials.defaultSubscriptionId, }, }; @@ -144,7 +141,6 @@ export function updateCredentials( cloudName: credentials.azureCloud || getDefaultAzureCloud(), tenantId: credentials.tenantId, clientId: credentials.clientId, - subscriptionId: credentials.defaultSubscriptionId, }, secureJsonData: { ...options.secureJsonData, diff --git a/public/app/plugins/datasource/grafana-azure-monitor-datasource/types/types.ts b/public/app/plugins/datasource/grafana-azure-monitor-datasource/types/types.ts index e3757299436..7cdee2efe54 100644 --- a/public/app/plugins/datasource/grafana-azure-monitor-datasource/types/types.ts +++ b/public/app/plugins/datasource/grafana-azure-monitor-datasource/types/types.ts @@ -35,7 +35,6 @@ export type ConcealedSecret = symbol; interface AzureCredentialsBase { authType: AzureAuthType; - defaultSubscriptionId?: string; } export interface AzureManagedIdentityCredentials extends AzureCredentialsBase {