From 169a1c5b0fa6ae22a4d755a0168186274f70ac9f Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Wed, 13 Jul 2022 11:50:20 +0200 Subject: [PATCH] Use non-blocking way of fetching ruler api availability (#52102) Co-authored-by: Joe Blubaugh --- .betterer.results | 3 -- .../unified/components/rules/CloudRules.tsx | 5 +-- .../components/rules/RulesGroup.test.tsx | 30 ++++++++++------ .../unified/components/rules/RulesGroup.tsx | 5 +-- .../unified/components/rules/RulesTable.tsx | 8 ++--- .../alerting/unified/hooks/useHasRuler.ts | 19 +++++++++-- .../alerting/unified/state/actions.ts | 34 +++++++++++-------- 7 files changed, 65 insertions(+), 39 deletions(-) diff --git a/.betterer.results b/.betterer.results index bc6732cb55f29..50522743d2bf9 100644 --- a/.betterer.results +++ b/.betterer.results @@ -3910,9 +3910,6 @@ exports[`better eslint`] = { [0, 0, 0, "Do not use any type assertions.", "0"], [0, 0, 0, "Do not use any type assertions.", "1"] ], - "public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx:5381": [ - [0, 0, 0, "Unexpected any. Specify a different type.", "0"] - ], "public/app/features/alerting/unified/components/silences/SilencesEditor.tsx:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"] ], diff --git a/public/app/features/alerting/unified/components/rules/CloudRules.tsx b/public/app/features/alerting/unified/components/rules/CloudRules.tsx index 018bf91ea62b5..982a2fc934635 100644 --- a/public/app/features/alerting/unified/components/rules/CloudRules.tsx +++ b/public/app/features/alerting/unified/components/rules/CloudRules.tsx @@ -23,13 +23,14 @@ interface Props { export const CloudRules: FC = ({ namespaces, expandAll }) => { const styles = useStyles2(getStyles); + const dsConfigs = useUnifiedAlertingSelector((state) => state.dataSources); const rules = useUnifiedAlertingSelector((state) => state.promRules); const rulesDataSources = useMemo(getRulesDataSources, []); const groupsWithNamespaces = useCombinedGroupNamespace(namespaces); const dataSourcesLoading = useMemo( - () => rulesDataSources.filter((ds) => rules[ds.name]?.loading), - [rules, rulesDataSources] + () => rulesDataSources.filter((ds) => rules[ds.name]?.loading || dsConfigs[ds.name]?.loading), + [rules, dsConfigs, rulesDataSources] ); const { numberOfPages, onPageChange, page, pageItems } = usePagination( diff --git a/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx b/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx index 6a0fb892c4054..cccbab9b0e307 100644 --- a/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx +++ b/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx @@ -8,17 +8,26 @@ import { contextSrv } from 'app/core/services/context_srv'; import { configureStore } from 'app/store/configureStore'; import { CombinedRuleGroup, CombinedRuleNamespace } from 'app/types/unified-alerting'; +import { useHasRuler } from '../../hooks/useHasRuler'; import { disableRBAC, mockCombinedRule, mockDataSource } from '../../mocks'; import { RulesGroup } from './RulesGroup'; -const hasRulerMock = jest.fn(); -jest.mock('../../hooks/useHasRuler', () => ({ - useHasRuler: () => hasRulerMock, -})); +jest.mock('../../hooks/useHasRuler'); + +const mocks = { + useHasRuler: jest.mocked(useHasRuler), +}; + +function mockUseHasRuler(hasRuler: boolean, rulerRulesLoaded: boolean) { + mocks.useHasRuler.mockReturnValue({ + hasRuler: () => hasRuler, + rulerRulesLoaded: () => rulerRulesLoaded, + }); +} beforeEach(() => { - hasRulerMock.mockReset(); + mocks.useHasRuler.mockReset(); }); const ui = { @@ -55,6 +64,7 @@ describe('Rules group tests', () => { it('Should hide delete and edit group buttons', () => { // Act + mockUseHasRuler(true, true); renderRulesGroup(namespace, group); // Assert @@ -83,33 +93,33 @@ describe('Rules group tests', () => { it('When ruler enabled should display delete and edit group buttons', () => { // Arrange - hasRulerMock.mockReturnValue(true); + mockUseHasRuler(true, true); // Act renderRulesGroup(namespace, group); // Assert - expect(hasRulerMock).toHaveBeenCalled(); + expect(mocks.useHasRuler).toHaveBeenCalled(); expect(ui.deleteGroupButton.get()).toBeInTheDocument(); expect(ui.editGroupButton.get()).toBeInTheDocument(); }); it('When ruler disabled should hide delete and edit group buttons', () => { // Arrange - hasRulerMock.mockReturnValue(false); + mockUseHasRuler(false, false); // Act renderRulesGroup(namespace, group); // Assert - expect(hasRulerMock).toHaveBeenCalled(); + expect(mocks.useHasRuler).toHaveBeenCalled(); expect(ui.deleteGroupButton.query()).not.toBeInTheDocument(); expect(ui.editGroupButton.query()).not.toBeInTheDocument(); }); it('Delete button click should display confirmation modal', async () => { // Arrange - hasRulerMock.mockReturnValue(true); + mockUseHasRuler(true, true); // Act renderRulesGroup(namespace, group); diff --git a/public/app/features/alerting/unified/components/rules/RulesGroup.tsx b/public/app/features/alerting/unified/components/rules/RulesGroup.tsx index be9c29a91f845..65f6e9b0284bb 100644 --- a/public/app/features/alerting/unified/components/rules/RulesGroup.tsx +++ b/public/app/features/alerting/unified/components/rules/RulesGroup.tsx @@ -43,13 +43,14 @@ export const RulesGroup: FC = React.memo(({ group, namespace, expandAll } setIsCollapsed(!expandAll); }, [expandAll]); - const hasRuler = useHasRuler(); + const { hasRuler, rulerRulesLoaded } = useHasRuler(); const rulerRule = group.rules[0]?.rulerRule; const folderUID = (rulerRule && isGrafanaRulerRule(rulerRule) && rulerRule.grafana_alert.namespace_uid) || undefined; const { folder } = useFolder(folderUID); // group "is deleting" if rules source has ruler, but this group has no rules that are in ruler - const isDeleting = hasRuler(rulesSource) && !group.rules.find((rule) => !!rule.rulerRule); + const isDeleting = + hasRuler(rulesSource) && rulerRulesLoaded(rulesSource) && !group.rules.find((rule) => !!rule.rulerRule); const isFederated = isFederatedRuleGroup(group); const deleteGroup = () => { diff --git a/public/app/features/alerting/unified/components/rules/RulesTable.tsx b/public/app/features/alerting/unified/components/rules/RulesTable.tsx index 6bc2dd398bb23..ffd44e3f1d23d 100644 --- a/public/app/features/alerting/unified/components/rules/RulesTable.tsx +++ b/public/app/features/alerting/unified/components/rules/RulesTable.tsx @@ -106,7 +106,7 @@ export const getStyles = (theme: GrafanaTheme2) => ({ }); function useColumns(showSummaryColumn: boolean, showGroupColumn: boolean) { - const hasRuler = useHasRuler(); + const { hasRuler, rulerRulesLoaded } = useHasRuler(); return useMemo((): RuleTableColumnProps[] => { const columns: RuleTableColumnProps[] = [ @@ -118,8 +118,8 @@ function useColumns(showSummaryColumn: boolean, showGroupColumn: boolean) { const { namespace } = rule; const { rulesSource } = namespace; const { promRule, rulerRule } = rule; - const isDeleting = !!(hasRuler(rulesSource) && promRule && !rulerRule); - const isCreating = !!(hasRuler(rulesSource) && rulerRule && !promRule); + const isDeleting = !!(hasRuler(rulesSource) && rulerRulesLoaded(rulesSource) && promRule && !rulerRule); + const isCreating = !!(hasRuler(rulesSource) && rulerRulesLoaded(rulesSource) && rulerRule && !promRule); return ; }, size: '165px', @@ -188,5 +188,5 @@ function useColumns(showSummaryColumn: boolean, showGroupColumn: boolean) { }); } return columns; - }, [hasRuler, showSummaryColumn, showGroupColumn]); + }, [hasRuler, rulerRulesLoaded, showSummaryColumn, showGroupColumn]); } diff --git a/public/app/features/alerting/unified/hooks/useHasRuler.ts b/public/app/features/alerting/unified/hooks/useHasRuler.ts index fb4d2e5960ce7..d9451d4bb86ce 100644 --- a/public/app/features/alerting/unified/hooks/useHasRuler.ts +++ b/public/app/features/alerting/unified/hooks/useHasRuler.ts @@ -2,18 +2,31 @@ import { useCallback } from 'react'; import { RulesSource } from 'app/types/unified-alerting'; -import { GRAFANA_RULES_SOURCE_NAME } from '../utils/datasource'; +import { getRulesSourceName, GRAFANA_RULES_SOURCE_NAME } from '../utils/datasource'; import { useUnifiedAlertingSelector } from './useUnifiedAlertingSelector'; // datasource has ruler if it's grafana managed or if we're able to load rules from it -export function useHasRuler(): (rulesSource: string | RulesSource) => boolean { +export function useHasRuler() { const rulerRules = useUnifiedAlertingSelector((state) => state.rulerRules); - return useCallback( + + const hasRuler = useCallback( (rulesSource: string | RulesSource) => { const rulesSourceName = typeof rulesSource === 'string' ? rulesSource : rulesSource.name; return rulesSourceName === GRAFANA_RULES_SOURCE_NAME || !!rulerRules[rulesSourceName]?.result; }, [rulerRules] ); + + const rulerRulesLoaded = useCallback( + (rulesSource: RulesSource) => { + const rulesSourceName = getRulesSourceName(rulesSource); + const result = rulerRules[rulesSourceName]?.result; + + return Boolean(result); + }, + [rulerRules] + ); + + return { hasRuler, rulerRulesLoaded }; } diff --git a/public/app/features/alerting/unified/state/actions.ts b/public/app/features/alerting/unified/state/actions.ts index 6f32eabb1f350..2109a8ac98b2b 100644 --- a/public/app/features/alerting/unified/state/actions.ts +++ b/public/app/features/alerting/unified/state/actions.ts @@ -255,30 +255,34 @@ export const fetchRulesSourceBuildInfoAction = createAsyncThunk( const dataSources: AsyncRequestMapSlice = (getState() as StoreState).unifiedAlerting .dataSources; const hasLoaded = Boolean(dataSources[rulesSourceName]?.result); - return !hasLoaded; + const hasError = Boolean(dataSources[rulesSourceName]?.error); + + return !(hasLoaded || hasError); }, } ); export function fetchAllPromAndRulerRulesAction(force = false): ThunkResult { return async (dispatch, getStore) => { - await dispatch(fetchAllPromBuildInfoAction()); + return Promise.all( + getAllRulesSourceNames().map(async (rulesSourceName) => { + await dispatch(fetchRulesSourceBuildInfoAction({ rulesSourceName })); - const { promRules, rulerRules, dataSources } = getStore().unifiedAlerting; + const { promRules, rulerRules, dataSources } = getStore().unifiedAlerting; + const dataSourceConfig = dataSources[rulesSourceName].result; - getAllRulesSourceNames().map((rulesSourceName) => { - const dataSourceConfig = dataSources[rulesSourceName].result; - if (!dataSourceConfig) { - return; - } + if (!dataSourceConfig) { + return; + } - if (force || !promRules[rulesSourceName]?.loading) { - dispatch(fetchPromRulesAction({ rulesSourceName })); - } - if ((force || !rulerRules[rulesSourceName]?.loading) && dataSourceConfig.rulerConfig) { - dispatch(fetchRulerRulesAction({ rulesSourceName })); - } - }); + if (force || !promRules[rulesSourceName]?.loading) { + dispatch(fetchPromRulesAction({ rulesSourceName })); + } + if ((force || !rulerRules[rulesSourceName]?.loading) && dataSourceConfig.rulerConfig) { + dispatch(fetchRulerRulesAction({ rulesSourceName })); + } + }) + ); }; }