-
Notifications
You must be signed in to change notification settings - Fork 73
WIP: [UC Backup] BackupStorageLocation UX improvements #7972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
199d31c
649c51f
0fcd91f
d1335ea
30e2fa4
97085f7
0f3bafe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,8 @@ | |||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import {ChangeDetectorRef, Component, Input, OnDestroy, OnInit} from '@angular/core'; | ||||||||||||||||||||||||||||||||||||||||||
| import {FormBuilder, FormControl, FormGroup, Validators} from '@angular/forms'; | ||||||||||||||||||||||||||||||||||||||||||
| import {MatDialogRef} from '@angular/material/dialog'; | ||||||||||||||||||||||||||||||||||||||||||
| import {MatDialog, MatDialogRef} from '@angular/material/dialog'; | ||||||||||||||||||||||||||||||||||||||||||
| import {MatSelectChange} from '@angular/material/select'; | ||||||||||||||||||||||||||||||||||||||||||
| import {ClusterBackupService} from '@app/core/services/cluster-backup'; | ||||||||||||||||||||||||||||||||||||||||||
| import {UserClusterConfigService} from '@app/core/services/user-cluster-config'; | ||||||||||||||||||||||||||||||||||||||||||
| import {DynamicModule} from '@app/dynamic/module-registry'; | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -58,7 +59,7 @@ import {IPV4_IPV6_CIDR_PATTERN} from '@shared/validators/others'; | |||||||||||||||||||||||||||||||||||||||||
| import {KmValidators} from '@shared/validators/validators'; | ||||||||||||||||||||||||||||||||||||||||||
| import _ from 'lodash'; | ||||||||||||||||||||||||||||||||||||||||||
| import {Observable, Subject} from 'rxjs'; | ||||||||||||||||||||||||||||||||||||||||||
| import {map, switchMap, take, takeUntil, tap} from 'rxjs/operators'; | ||||||||||||||||||||||||||||||||||||||||||
| import {filter, map, switchMap, take, takeUntil, tap} from 'rxjs/operators'; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| enum Controls { | ||||||||||||||||||||||||||||||||||||||||||
| Name = 'name', | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -127,6 +128,7 @@ export class EditClusterComponent implements OnInit, OnDestroy { | |||||||||||||||||||||||||||||||||||||||||
| enforcedAuditWebhookSettings: AuditLoggingWebhookBackend; | ||||||||||||||||||||||||||||||||||||||||||
| backupStorageLocationsList: BackupStorageLocation[]; | ||||||||||||||||||||||||||||||||||||||||||
| backupStorageLocationLabel: BSLListState = BSLListState.Ready; | ||||||||||||||||||||||||||||||||||||||||||
| readonly createBackupStorageLocationOptionValue = '__create_backup_storage_location__'; | ||||||||||||||||||||||||||||||||||||||||||
| isAllowedIPRangeSupported: boolean; | ||||||||||||||||||||||||||||||||||||||||||
| readonly isEnterpriseEdition = DynamicModule.isEnterpriseEdition; | ||||||||||||||||||||||||||||||||||||||||||
| readonly CLUSTER_DEFAULT_NODE_SELECTOR_NAMESPACE = CLUSTER_DEFAULT_NODE_SELECTOR_NAMESPACE; | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -157,6 +159,7 @@ export class EditClusterComponent implements OnInit, OnDestroy { | |||||||||||||||||||||||||||||||||||||||||
| private readonly _builder: FormBuilder, | ||||||||||||||||||||||||||||||||||||||||||
| private readonly _clusterService: ClusterService, | ||||||||||||||||||||||||||||||||||||||||||
| private readonly _datacenterService: DatacenterService, | ||||||||||||||||||||||||||||||||||||||||||
| private readonly _matDialog: MatDialog, | ||||||||||||||||||||||||||||||||||||||||||
| private readonly _matDialogRef: MatDialogRef<EditClusterComponent>, | ||||||||||||||||||||||||||||||||||||||||||
| private readonly _notificationService: NotificationService, | ||||||||||||||||||||||||||||||||||||||||||
| private readonly _settingsService: SettingsService, | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -529,11 +532,48 @@ export class EditClusterComponent implements OnInit, OnDestroy { | |||||||||||||||||||||||||||||||||||||||||
| .listBackupStorageLocation(projectID) | ||||||||||||||||||||||||||||||||||||||||||
| .pipe(takeUntil(this._unsubscribe)) | ||||||||||||||||||||||||||||||||||||||||||
| .subscribe(cbslList => { | ||||||||||||||||||||||||||||||||||||||||||
| this.backupStorageLocationsList = cbslList; | ||||||||||||||||||||||||||||||||||||||||||
| this.backupStorageLocationLabel = cbslList.length ? BSLListState.Ready : BSLListState.Empty; | ||||||||||||||||||||||||||||||||||||||||||
| this.backupStorageLocationsList = cbslList.filter(bsl => this._isBackupStorageLocationAvailable(bsl)); | ||||||||||||||||||||||||||||||||||||||||||
| this.backupStorageLocationLabel = this.backupStorageLocationsList.length | ||||||||||||||||||||||||||||||||||||||||||
| ? BSLListState.Ready | ||||||||||||||||||||||||||||||||||||||||||
| : BSLListState.Empty; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const backupStorageLocationControl = this.form.get(Controls.BackupStorageLocation); | ||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||
| backupStorageLocationControl && | ||||||||||||||||||||||||||||||||||||||||||
| !this.backupStorageLocationsList.some(bsl => bsl.name === backupStorageLocationControl.value) | ||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||
| backupStorageLocationControl.reset(); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| async onBackupStorageLocationSelectionChange(event: MatSelectChange<string>): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||
| if (event.value !== this.createBackupStorageLocationOptionValue) { | ||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const backupStorageLocationControl = this.form.get(Controls.BackupStorageLocation); | ||||||||||||||||||||||||||||||||||||||||||
| const previousValue = this.cluster.spec?.backupConfig?.backupStorageLocation?.name ?? ''; | ||||||||||||||||||||||||||||||||||||||||||
| backupStorageLocationControl.setValue(previousValue, {emitEvent: false}); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const AddBackupStorageLocationDialogComponent = await DynamicModule.AddBackupStorageLocationDialogComponent; | ||||||||||||||||||||||||||||||||||||||||||
| if (!AddBackupStorageLocationDialogComponent) { | ||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| this._matDialog | ||||||||||||||||||||||||||||||||||||||||||
| .open(AddBackupStorageLocationDialogComponent, { | ||||||||||||||||||||||||||||||||||||||||||
| data: {projectID: this.projectID}, | ||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||
| .afterClosed() | ||||||||||||||||||||||||||||||||||||||||||
| .pipe(take(1), filter(Boolean)) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+558
to
+568
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like this would be more consistent with the rest of the code.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
| .subscribe(() => this._getCBSL(this.projectID)); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| private _isBackupStorageLocationAvailable(bsl: BackupStorageLocation): boolean { | ||||||||||||||||||||||||||||||||||||||||||
| const status = bsl.status?.phase || bsl.spec?.status || ''; | ||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't think we need to check for status cause it will never be
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
| return status.toLowerCase() === 'available'; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| getObservable(): Observable<Cluster> { | ||||||||||||||||||||||||||||||||||||||||||
| const patch: ClusterPatch = { | ||||||||||||||||||||||||||||||||||||||||||
| name: this.form.get(Controls.Name).value, | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |||||
| // END OF TERMS AND CONDITIONS | ||||||
|
|
||||||
| import {Component, Inject, OnDestroy, OnInit} from '@angular/core'; | ||||||
| import {FormBuilder, FormGroup, Validators} from '@angular/forms'; | ||||||
| import {AbstractControl, FormBuilder, FormGroup, ValidationErrors, ValidatorFn, Validators} from '@angular/forms'; | ||||||
| import {MAT_DIALOG_DATA, MatDialogRef} from '@angular/material/dialog'; | ||||||
| import {ClusterBackupService} from '@app/core/services/cluster-backup'; | ||||||
| import {NotificationService} from '@app/core/services/notification'; | ||||||
|
|
@@ -64,6 +64,7 @@ enum Controls { | |||||
| }) | ||||||
| export class AddBackupStorageLocationDialogComponent implements OnInit, OnDestroy { | ||||||
| private readonly _unsubscribe = new Subject<void>(); | ||||||
| private readonly _dnsLabelRegex = /^[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?$/i; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be change to include the to the |
||||||
| readonly Controls = Controls; | ||||||
| readonly veleroChecksumAlgorithms = Object.values(VeleroChecksumAlgorithm); | ||||||
| form: FormGroup; | ||||||
|
|
@@ -108,12 +109,18 @@ export class AddBackupStorageLocationDialogComponent implements OnInit, OnDestro | |||||
| this._config.bslObject?.spec.backupSyncPeriod ?? '0', | ||||||
| CBSL_SYNC_PERIOD | ||||||
| ), | ||||||
| [Controls.Region]: this._builder.control(this._config.bslObject?.spec.config?.region ?? ''), | ||||||
| [Controls.Endpoints]: this._builder.control(this._config.bslObject?.spec.config?.s3Url ?? ''), | ||||||
| [Controls.Region]: this._builder.control(this._config.bslObject?.spec.config?.region ?? '', [ | ||||||
| this._dnsNameValidator(), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ]), | ||||||
| [Controls.Endpoints]: this._builder.control(this._config.bslObject?.spec.config?.s3Url ?? '', [ | ||||||
| this._endpointURLValidator(), | ||||||
| ]), | ||||||
| [Controls.ChecksumAlgorithm]: this._builder.control(this._config.bslObject?.spec.config?.checksumAlgorithm ?? ''), | ||||||
| [Controls.AddCustomConfig]: this._builder.control(false), | ||||||
| }); | ||||||
|
|
||||||
| this._updateRegionAndEndpointValidators(this.form.get(Controls.AddCustomConfig).value); | ||||||
|
|
||||||
| if (this._config.bslObject) { | ||||||
| this.form.get(Controls.Name).disable(); | ||||||
| } else { | ||||||
|
|
@@ -132,6 +139,7 @@ export class AddBackupStorageLocationDialogComponent implements OnInit, OnDestro | |||||
| .get(Controls.AddCustomConfig) | ||||||
| .valueChanges.pipe(takeUntil(this._unsubscribe)) | ||||||
| .subscribe((value: boolean) => { | ||||||
| this._updateRegionAndEndpointValidators(value); | ||||||
| let config: BackupStorageLocationConfig; | ||||||
| if (this._config.bslObject?.name) { | ||||||
| config = this._config.bslObject.spec.config; | ||||||
|
|
@@ -196,7 +204,7 @@ export class AddBackupStorageLocationDialogComponent implements OnInit, OnDestro | |||||
| this.form.get(Controls.BackupSyncPeriod).value === '' ? null : this.form.get(Controls.BackupSyncPeriod).value, | ||||||
| config: { | ||||||
| region: this.form.get(Controls.Region).value, | ||||||
| s3Url: this.form.get(Controls.Endpoints).value, | ||||||
| s3Url: this.form.get(Controls.Endpoints).value?.trim(), | ||||||
| checksumAlgorithm: this.form.get(Controls.ChecksumAlgorithm).value || '', | ||||||
| }, | ||||||
| provider: SupportedBSLProviders.AWS, | ||||||
|
|
@@ -217,4 +225,64 @@ export class AddBackupStorageLocationDialogComponent implements OnInit, OnDestro | |||||
| } | ||||||
| return bsl; | ||||||
| } | ||||||
|
|
||||||
| private _dnsNameValidator(): ValidatorFn { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually add validators in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do have a file let me wdyt?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well after another look i don't think we need this method just to repeat the same regex check. |
||||||
| return (control: AbstractControl): ValidationErrors | null => { | ||||||
| const value = control.value?.trim(); | ||||||
| if (!value) { | ||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| const labels = value.split('.'); | ||||||
| return labels.every(label => this._dnsLabelRegex.test(label)) ? null : {invalidDnsName: true}; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to double confirm? values like
are not valid DNS |
||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| private _endpointURLValidator(): ValidatorFn { | ||||||
| return (control: AbstractControl): ValidationErrors | null => { | ||||||
| const value = control.value?.trim(); | ||||||
| if (!value) { | ||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| if (!/^https?:\/\//i.test(value)) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to double confirm if following values are not valid test value?
|
||||||
| return {invalidEndpointUrl: true}; | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| const url = new URL(value); | ||||||
| if (!['http:', 'https:'].includes(url.protocol)) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think if we have this check then we don't need the the previous condition |
||||||
| return {invalidEndpointUrl: true}; | ||||||
| } | ||||||
|
|
||||||
| if (!url.hostname) { | ||||||
| return {invalidEndpointUrl: true}; | ||||||
| } | ||||||
|
|
||||||
| const labels = url.hostname.split('.'); | ||||||
| return labels.every(label => this._dnsLabelRegex.test(label)) ? null : {invalidEndpointUrl: true}; | ||||||
| } catch { | ||||||
| return {invalidEndpointUrl: true}; | ||||||
| } | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| private _updateRegionAndEndpointValidators(addCustomConfig: boolean): void { | ||||||
| const regionControl = this.form.get(Controls.Region); | ||||||
| const endpointControl = this.form.get(Controls.Endpoints); | ||||||
|
|
||||||
| const regionValidators = [this._dnsNameValidator()]; | ||||||
| const endpointValidators = [this._endpointURLValidator()]; | ||||||
|
|
||||||
| if (!addCustomConfig) { | ||||||
| regionValidators.unshift(Validators.required); | ||||||
| endpointValidators.unshift(Validators.required); | ||||||
| } | ||||||
|
|
||||||
| regionControl.setValidators(regionValidators); | ||||||
| endpointControl.setValidators(endpointValidators); | ||||||
|
|
||||||
| regionControl.updateValueAndValidity({emitEvent: false}); | ||||||
| endpointControl.updateValueAndValidity({emitEvent: false}); | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async/await is working fine, but since the rest of the component uses RxJS pipelines, I think it’s better to stay consistent with that approach.