Skip to content

Commit 8244e3f

Browse files
AbhishekA1509AbhishekA1509
authored
Merge pull request #2939 from devtron-labs/fix/cluster-form-error
fix: cluster form error
2 parents 7fd8ddf + 58e263e commit 8244e3f

File tree

7 files changed

+136
-86
lines changed

7 files changed

+136
-86
lines changed

‎src/Pages/GlobalConfigurations/ClustersAndEnvironments/ClusterForm/ApplicationMonitoring.tsx‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,12 @@ import { ApplicationMonitoringProps } from './types'
66
const ApplicationMonitoring = ({
77
prometheusConfig,
88
prometheusUrl,
9-
prometheusToggleEnabled,
10-
setPrometheusToggle,
9+
isAppMetricsEnabled,
10+
toggleAppMetrics,
1111
handleOnChange,
1212
onPrometheusAuthTypeChange,
1313
isGrafanaModuleInstalled,
14+
isCostVisibilityEnabled,
1415
}: ApplicationMonitoringProps) => (
1516
<div className="flexbox-col flex-grow-1 dc__gap-20 p-20">
1617
<div className="flexbox-col">
@@ -29,13 +30,18 @@ const ApplicationMonitoring = ({
2930
<DTSwitch
3031
name="toggle-configure-prometheus"
3132
ariaLabel="Toggle configure prometheus"
32-
isChecked={prometheusToggleEnabled}
33-
onChange={setPrometheusToggle}
34-
isDisabled={!isGrafanaModuleInstalled}
33+
isChecked={isAppMetricsEnabled}
34+
onChange={toggleAppMetrics}
35+
isDisabled={!isGrafanaModuleInstalled || (isAppMetricsEnabled && isCostVisibilityEnabled)}
36+
tooltipContent={
37+
isAppMetricsEnabled && isCostVisibilityEnabled
38+
? 'To disable application metrics, first disable cost visibility from cluster cost configuration.'
39+
: ''
40+
}
3541
/>
3642
</div>
37-
{!prometheusToggleEnabled && prometheusUrl && <PrometheusWarningInfo />}
38-
{prometheusToggleEnabled && (
43+
{!isAppMetricsEnabled && prometheusUrl && <PrometheusWarningInfo />}
44+
{isAppMetricsEnabled && (
3945
<PromoetheusConfigCard
4046
prometheusConfig={prometheusConfig}
4147
handleOnChange={handleOnChange}

‎src/Pages/GlobalConfigurations/ClustersAndEnvironments/ClusterForm/ClusterForm.tsx‎

Lines changed: 81 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,15 @@ import {
2424
ButtonVariantType,
2525
ClusterCostModuleConfigPayload,
2626
ClusterDetailListType,
27+
ClusterProviderType,
2728
DEFAULT_SECRET_PLACEHOLDER,
2829
Icon,
2930
ModalSidebarPanel,
3031
ModuleNameMap,
3132
ModuleStatus,
3233
noop,
3334
RemoteConnectionType,
35+
SCHEMA_07_VALIDATOR,
3436
SelectPickerOptionType,
3537
showError,
3638
ToastManager,
@@ -83,6 +85,7 @@ const ClusterForm = ({
8385
category,
8486
clusterProvider,
8587
costModuleConfig,
88+
costModuleSchema,
8689
}: ClusterFormProps) => {
8790
const location = useLocation()
8891

@@ -93,14 +96,19 @@ const ClusterForm = ({
9396
)
9497

9598
const [costModuleState, setCostModuleState] = useState<
96-
Pick<ClusterDetailListType['costModuleConfig'], 'enabled'> & { config: string }
99+
Pick<ClusterDetailListType['costModuleConfig'], 'enabled'> & {
100+
config: ClusterDetailListType['costModuleConfig']['config'] & { provider: ClusterProviderType }
101+
}
97102
>({
98103
enabled: costModuleConfig?.enabled || false,
99-
config: costModuleConfig?.config ? JSON.stringify(costModuleConfig.config) : '',
104+
config: {
105+
...costModuleConfig?.config,
106+
// Adding provider here to make the form work, it won't be sent to backend
107+
provider: clusterProvider,
108+
},
100109
})
101-
const [costModuleConfigErrorState, setCostModuleErrorState] = useState<string>('')
102110

103-
const [prometheusToggleEnabled, setPrometheusToggleEnabled] = useState(!!prometheusUrl)
111+
const [isAppMetricsEnabled, setIsAppMetricsEnabled] = useState(!!prometheusUrl)
104112
const [prometheusAuthenticationType, setPrometheusAuthenticationType] = useState({
105113
type: prometheusAuth?.userName ? AuthenticationType.BASIC : AuthenticationType.ANONYMOUS,
106114
})
@@ -111,6 +119,8 @@ const ClusterForm = ({
111119
const [isConnectedViaProxyTemp, setIsConnectedViaProxyTemp] = useState(!!proxyUrl)
112120
const [isConnectedViaSSHTunnelTemp, setIsConnectedViaSSHTunnelTemp] = useState(isConnectedViaSSHTunnel)
113121

122+
const isPrometheusEnabled = costModuleState.enabled || isAppMetricsEnabled
123+
114124
const getRemoteConnectionConfigType = () => {
115125
if (isConnectedViaProxyTemp) {
116126
return RemoteConnectionType.Proxy
@@ -179,28 +189,20 @@ const ClusterForm = ({
179189
setIsConnectedViaSSHTunnelTemp(false)
180190
}
181191

182-
const validateCostModuleConfig = (requiredConfig: string = costModuleState.config): string => {
183-
try {
184-
if (requiredConfig) {
185-
JSON.parse(requiredConfig)
186-
}
187-
188-
return ''
189-
} catch (e) {
190-
return e.message || 'Invalid JSON'
192+
const validateCostModuleConfig = (): boolean => {
193+
if (!costModuleSchema || !costModuleState.enabled) {
194+
return true
191195
}
192-
}
193196

194-
const getParsedConfigValue = (): ClusterCostModuleConfigPayload['config'] => {
195-
if (costModuleState.config) {
196-
try {
197-
const parsedConfig = JSON.parse(costModuleState.config)
198-
return parsedConfig
199-
} catch {
200-
return {}
201-
}
197+
try {
198+
const validationResult = SCHEMA_07_VALIDATOR.validateFormData(
199+
{ ...costModuleState.config },
200+
costModuleSchema,
201+
)
202+
return !validationResult?.errors?.length
203+
} catch {
204+
return true
202205
}
203-
return null
204206
}
205207

206208
const getCostModulePayload = (): ClusterCostModuleConfigPayload | null => {
@@ -211,9 +213,12 @@ const ClusterForm = ({
211213
}
212214

213215
if (costModuleState.config) {
216+
// eslint-disable-next-line @typescript-eslint/no-unused-vars
217+
const { provider, ...sanitizedConfig } = costModuleState.config
218+
214219
return {
215220
enabled: true,
216-
config: getParsedConfigValue(),
221+
config: sanitizedConfig,
217222
}
218223
}
219224

@@ -236,25 +241,31 @@ const ClusterForm = ({
236241
isProd: state.isProd.value === 'true',
237242
active: true,
238243
remoteConnectionConfig: getRemoteConnectionConfig(state, remoteConnectionMethod, SSHConnectionType),
239-
prometheus_url: prometheusToggleEnabled ? state.endpoint.value : '',
244+
prometheus_url: isPrometheusEnabled ? state.endpoint.value : '',
240245
prometheusAuth: {
241246
userName:
242-
prometheusToggleEnabled && state.authType.value === AuthenticationType.BASIC
243-
? state.userName.value
244-
: '',
247+
isPrometheusEnabled && state.authType.value === AuthenticationType.BASIC ? state.userName.value : '',
245248
password:
246-
prometheusToggleEnabled && state.authType.value === AuthenticationType.BASIC
247-
? state.password.value
248-
: '',
249-
tlsClientKey: prometheusToggleEnabled ? state.prometheusTlsClientKey.value : '',
250-
tlsClientCert: prometheusToggleEnabled ? state.prometheusTlsClientCert.value : '',
249+
isPrometheusEnabled && state.authType.value === AuthenticationType.BASIC ? state.password.value : '',
250+
tlsClientKey: isPrometheusEnabled ? state.prometheusTlsClientKey.value : '',
251+
tlsClientCert: isPrometheusEnabled ? state.prometheusTlsClientCert.value : '',
251252
isAnonymous: state.authType.value === AuthenticationType.ANONYMOUS,
252253
},
253254
server_url: '',
254255
...(getCategoryPayload ? getCategoryPayload(selectedCategory) : null),
255-
...(clusterProvider ? { costModuleConfig: getCostModulePayload() } : null),
256+
...(costModuleSchema ? { costModuleConfig: getCostModulePayload() } : null),
256257
})
257258

259+
const additionalValidations = (): boolean => {
260+
let hasError = false
261+
262+
if (!validateCostModuleConfig()) {
263+
hasError = true
264+
}
265+
266+
return hasError
267+
}
268+
258269
const onValidation = async (state) => {
259270
const payload = getClusterPayload(state)
260271
const urlValue = state.url.value?.trim() ?? ''
@@ -264,16 +275,15 @@ const ClusterForm = ({
264275
payload.server_url = urlValue
265276
}
266277

267-
if (costModuleState.enabled) {
268-
const costConfigError = validateCostModuleConfig()
269-
if (costConfigError) {
270-
setCostModuleErrorState(costConfigError)
271-
ToastManager.showToast({
272-
variant: ToastVariantType.error,
273-
description: 'Invalid cost visibility configuration',
274-
})
275-
return
276-
}
278+
const hasAdditionalError = additionalValidations()
279+
280+
if (hasAdditionalError) {
281+
ToastManager.showToast({
282+
variant: ToastVariantType.error,
283+
description: 'Some fields are invalid. Please correct them and try again.',
284+
})
285+
286+
return
277287
}
278288

279289
if (remoteConnectionMethod === RemoteConnectionType.Proxy) {
@@ -286,7 +296,9 @@ const ClusterForm = ({
286296
}
287297
}
288298

289-
if (state.authType.value === AuthenticationType.BASIC && prometheusToggleEnabled) {
299+
// Not adding this block in additionalValidations since there seems to be no state its error
300+
// They are both required in useForm so don't know why we have checked it here
301+
if (state.authType.value === AuthenticationType.BASIC && isPrometheusEnabled) {
290302
const isValid = state.userName?.value && state.password?.value
291303
if (!isValid) {
292304
ToastManager.showToast({
@@ -328,7 +340,7 @@ const ClusterForm = ({
328340
}
329341
}
330342

331-
const { state, handleOnChange, handleOnSubmit } = useForm(
343+
const { state, handleOnChange, handleOnSubmit, validateAllAndSetErrors } = useForm(
332344
{
333345
cluster_name: { value: clusterName, error: '' },
334346
url: { value: !id ? getServerURLFromLocalStorage(serverUrl) : serverUrl, error: '' },
@@ -368,11 +380,11 @@ const ClusterForm = ({
368380
validator: { error: 'Authentication Type is required', regex: /^(?!\s*$).+/ },
369381
},
370382
userName: {
371-
required: !!(prometheusToggleEnabled && prometheusAuthenticationType.type === AuthenticationType.BASIC),
383+
required: !!(isPrometheusEnabled && prometheusAuthenticationType.type === AuthenticationType.BASIC),
372384
validator: { error: 'username is required', regex: /^(?!\s*$).+/ },
373385
},
374386
password: {
375-
required: !!(prometheusToggleEnabled && prometheusAuthenticationType.type === AuthenticationType.BASIC),
387+
required: !!(isPrometheusEnabled && prometheusAuthenticationType.type === AuthenticationType.BASIC),
376388
validator: { error: 'password is required', regex: /^(?!\s*$).+/ },
377389
},
378390
prometheusTlsClientKey: {
@@ -438,15 +450,16 @@ const ClusterForm = ({
438450
validator: { error: 'token is required', regex: /[^]+/ },
439451
},
440452
endpoint: {
441-
required: !!prometheusToggleEnabled,
453+
required: !!isPrometheusEnabled,
442454
validator: { error: 'endpoint is required', regex: /^.*$/ },
443455
},
444456
},
445457
onValidation,
458+
'Please resolve all the errors before submitting.',
446459
)
447460

448-
const setPrometheusToggle = () => {
449-
setPrometheusToggleEnabled(!prometheusToggleEnabled)
461+
const toggleAppMetrics = () => {
462+
setIsAppMetricsEnabled((prev) => !prev)
450463
}
451464

452465
const onPrometheusAuthTypeChange = (e) => {
@@ -478,6 +491,16 @@ const ClusterForm = ({
478491
const hideConfirmationModal = () => setConfirmation(false)
479492

480493
const getTabSwitchHandler = (tab: ClusterConfigTabEnum) => () => {
494+
// This works since there is no required field in any other tab except Cluster config tab, which on creation is the default tab
495+
const hasError = validateAllAndSetErrors() || additionalValidations()
496+
if (hasError) {
497+
ToastManager.showToast({
498+
variant: ToastVariantType.error,
499+
description: 'Some fields are invalid. Please correct them and try again.',
500+
})
501+
return
502+
}
503+
481504
setClusterConfigTab(tab)
482505
}
483506

@@ -488,14 +511,11 @@ const ClusterForm = ({
488511
}))
489512
}
490513

491-
const handleCostConfigChange = (newConfig: string) => {
514+
const handleCostConfigChange = (newConfig: typeof costModuleState.config) => {
492515
setCostModuleState((prev) => ({
493516
...prev,
494517
config: newConfig,
495518
}))
496-
497-
const error = validateCostModuleConfig(newConfig)
498-
setCostModuleErrorState(error)
499519
}
500520

501521
const renderFooter = () => (
@@ -568,11 +588,12 @@ const ClusterForm = ({
568588
<ApplicationMonitoring
569589
prometheusConfig={prometheusConfig}
570590
prometheusUrl={prometheusUrl}
571-
prometheusToggleEnabled={prometheusToggleEnabled}
572-
setPrometheusToggle={setPrometheusToggle}
591+
isAppMetricsEnabled={isAppMetricsEnabled}
592+
toggleAppMetrics={toggleAppMetrics}
573593
handleOnChange={handleOnChange}
574594
onPrometheusAuthTypeChange={onPrometheusAuthTypeChange}
575595
isGrafanaModuleInstalled={isGrafanaModuleInstalled}
596+
isCostVisibilityEnabled={costModuleState.enabled}
576597
/>
577598
</div>
578599
)
@@ -587,10 +608,9 @@ const ClusterForm = ({
587608
costModuleEnabled={costModuleState.enabled}
588609
toggleCostModule={toggleCostModule}
589610
installationStatus={costModuleConfig.installationStatus}
590-
clusterProvider={clusterProvider}
591611
handleCostConfigChange={handleCostConfigChange}
592-
config={costModuleState.config || ''}
593-
configError={costModuleConfigErrorState}
612+
config={costModuleState.config}
613+
costModuleSchema={costModuleSchema || {}}
594614
/>
595615
</div>
596616
) : null
@@ -651,7 +671,7 @@ const ClusterForm = ({
651671
<ClusterFormNavButton
652672
isActive={clusterConfigTab === ClusterConfigTabEnum.APPLICATION_MONITORING}
653673
title="Application Monitoring"
654-
subtitle={prometheusToggleEnabled ? 'Enabled' : 'Off'}
674+
subtitle={isAppMetricsEnabled ? 'Enabled' : 'Off'}
655675
onClick={getTabSwitchHandler(ClusterConfigTabEnum.APPLICATION_MONITORING)}
656676
/>
657677
{ClusterCostConfig && id && (

‎src/Pages/GlobalConfigurations/ClustersAndEnvironments/ClusterForm/types.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,12 @@ export interface KubeConfigEditorProps {
3535
export interface ApplicationMonitoringProps {
3636
prometheusConfig: PromoetheusConfig
3737
prometheusUrl: string
38-
prometheusToggleEnabled: boolean
39-
setPrometheusToggle: () => void
38+
isAppMetricsEnabled: boolean
39+
toggleAppMetrics: () => void
4040
handleOnChange: (event: SyntheticEvent) => void
4141
onPrometheusAuthTypeChange: (event: SyntheticEvent) => void
4242
isGrafanaModuleInstalled: boolean
43+
isCostVisibilityEnabled: boolean
4344
}
4445

4546
export enum ClusterConfigTabEnum {

‎src/Pages/GlobalConfigurations/ClustersAndEnvironments/EditClusterDrawerContent.tsx‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,10 @@ const EditClusterDrawerContent = ({
5555
enabled: !!clusterId,
5656
})
5757

58-
const { prometheusAuthResult, clusterProvider } = metadata || {
58+
const { prometheusAuthResult, clusterProvider, costModuleSchema } = metadata || {
5959
prometheusAuthResult: null,
60-
cloudProvider: null,
60+
clusterProvider: null,
61+
costModuleSchema: null,
6162
}
6263

6364
return (
@@ -89,6 +90,7 @@ const EditClusterDrawerContent = ({
8990
installationId={installationId}
9091
category={category}
9192
clusterProvider={clusterProvider}
93+
costModuleSchema={costModuleSchema}
9294
costModuleConfig={costModuleConfig}
9395
/>
9496
</APIResponseHandler>

0 commit comments

Comments
 (0)