Skip to content
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

Implementation of 2966 bucket configure #36

Merged
merged 22 commits into from
Feb 16, 2023
Merged

Conversation

bcgv123
Copy link
Contributor

@bcgv123 bcgv123 commented Feb 14, 2023

This pr contains implementation of Bucket Configure feature. It includes 'Configure New Bucket' dialog and form as a separate components. Fields on form are generated by common TextInput component. TextInput supports field validation (required at the moment). The submit of the form is not allowed unless all the fields are populated. Update Bucket Configure is happening on wheel icon click and brings up the same dialog, but it loads the updating bucket into the fields instead of being empty.

Types of changes

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@github-actions
Copy link

Coverage Report (Application)

Totals Coverage
Statements: 75% ( 51 / 68 )
Methods: 62.5% ( 5 / 8 )
Lines: 82.61% ( 38 / 46 )
Branches: 57.14% ( 8 / 14 )

@bcgv123 bcgv123 force-pushed the feat/2966-bucket-configure branch from 0a0eca8 to c8860b7 Compare February 14, 2023 17:11
@github-actions
Copy link

Coverage Report (Frontend)

Totals Coverage
Statements: 43.33% ( 39 / 90 )
Methods: 20.83% ( 5 / 24 )
Lines: 52.83% ( 28 / 53 )
Branches: 46.15% ( 6 / 13 )

@loneil
Copy link
Contributor

loneil commented Feb 14, 2023

Discussed REDACTED field password write issue in Discord.

@loneil
Copy link
Contributor

loneil commented Feb 14, 2023

Could also consider validating bucket url to ensure it's URL
https://vee-validate.logaretm.com/v4/guide/global-validators#url

image

@kamorel
Copy link
Contributor

kamorel commented Feb 14, 2023

Could also consider validating bucket url to ensure it's URL https://vee-validate.logaretm.com/v4/guide/global-validators#url

Valid, but i'd prefer to have the FE yup validation schema match that of COMS which it is currently doing.

Comment on lines 41 to 54
const formBucket = {
bucketName: values.bucketName,
bucket: values.bucket,
endpoint: values.endpoint,
accessKeyId: values.accessKeyId,
secretAccessKey: values.secretAccessKey,
key: values.key
} as Bucket;

Copy link
Contributor

@kamorel kamorel Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use object spread syntax.
const formBucket: Bucket = { ...values };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kyle, here lint forces me to define the rest of the object. For example: region cannot be empty (getting error from the back-end) and don't have this info initialy.

@bcgv123 bcgv123 force-pushed the feat/2966-bucket-configure branch from 852e3be to 8dad7f6 Compare February 15, 2023 19:50
frontend/src/components/bucket/BucketConfigDialog.vue Outdated Show resolved Hide resolved
frontend/src/components/bucket/BucketConfigForm.vue Outdated Show resolved Hide resolved
frontend/src/components/bucket/BucketList.vue Outdated Show resolved Hide resolved
frontend/src/components/bucket/BucketsTable.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@TimCsaky TimCsaky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. I suspect we may want to revisit the decision to show 'REDACTED'.

frontend/src/components/form/Password.vue Show resolved Hide resolved
…ative path changed to an absolute, removed invalid attribute
@jujaga jujaga merged commit bbb79bd into master Feb 16, 2023
@jujaga jujaga deleted the feat/2966-bucket-configure branch February 16, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants