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

Package name validation #1998

Merged
merged 25 commits into from
Jan 15, 2021
Merged

Package name validation #1998

merged 25 commits into from
Jan 15, 2021

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented Jan 12, 2021

  • Package name error is shown immediately
  • Error message moved to helperText (text under Input)
  • Warning messages added to package name Input
  • Fixed bug with infinite spinner after losing focus

Warning messages:

Create package:

  • Package exists: "Package-name exists. Submitting will revise it"

Copy package:

Name is pre-filled and validating on start

  • Successor package exists: "Package-full-name exists. Submitting will revise it"
  • Successor package doesn't exist: "Package-full-name will be created"

Update package:

  • Change name and package doesn't exist: "New package will be created"
  • Change name but the package with the same name exists already: "Package with this name exists already"

Screenshot from 2021-01-12 19-20-33
Screenshot from 2021-01-12 19-20-52

@codecov-io
Copy link

codecov-io commented Jan 12, 2021

Codecov Report

Merging #1998 (998ea32) into master (3a06277) will decrease coverage by 0.20%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1998      +/-   ##
==========================================
- Coverage   46.68%   46.47%   -0.21%     
==========================================
  Files         332      332              
  Lines       16459    16532      +73     
  Branches     2195     2231      +36     
==========================================
  Hits         7684     7684              
- Misses       7876     7938      +62     
- Partials      899      910      +11     
Flag Coverage Δ
api-python 89.35% <ø> (ø)
catalog 7.32% <0.00%> (-0.07%) ⬇️
lambda 92.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
catalog/app/containers/Bucket/PackageCopyDialog.js 0.00% <0.00%> (ø)
...talog/app/containers/Bucket/PackageCreateDialog.js 0.00% <0.00%> (ø)
...p/containers/Bucket/PackageDialog/PackageDialog.js 0.00% <0.00%> (ø)
...talog/app/containers/Bucket/PackageUpdateDialog.js 0.00% <0.00%> (ø)
catalog/app/containers/Bucket/requests.js 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a06277...998ea32. Read the comment docs.

@fiskus fiskus requested review from nl0 and removed request for nl0 January 13, 2021 05:04
@fiskus fiskus changed the title Package name validation WIP: Package name validation Jan 13, 2021
@fiskus fiskus changed the title WIP: Package name validation Package name validation Jan 13, 2021
@fiskus fiskus requested a review from nl0 January 13, 2021 07:15
Copy link
Member

@nl0 nl0 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 in general, but we should change package existence checking code to check for package existence rather than data directory existence. plus some minor issues / questions in inline comments

endpoint: '/package_name_valid',
method: 'POST',
body: { name },
const list = await requests.bucketListing({
Copy link
Member

Choose a reason for hiding this comment

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

i think we should check for package existence here, not for data existence (either by querying the package index or listing the .quilt/named_packages/$handle dir)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know anything about this, please, provide more details

Copy link
Member

Choose a reason for hiding this comment

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

see the contents of .quilt/named_packages dir: it has all the packages registered in the bucket and has the $handle/$pointer_file structure, so to check if the package exists in the bucket you should list objects under .quilt/named_packages/$handle/ prefix -- if there are any, the package exists, otherwise it does not.

Copy link
Member Author

@fiskus fiskus Jan 14, 2021

Choose a reason for hiding this comment

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

I've added method requests.ensurePackageIsPresent, where i check for .quilt/named_packages/package_name/latest.

headObject(".quilt/named_packages/package_name") (just directory, without package revision) suddenly doesn't work

Copy link
Member

Choose a reason for hiding this comment

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

I've added method requests.ensurePackageIsPresent, where i check for .quilt/named_packages/package_name/latest.

i think listing revisions is more reliable, bc the latest tag may be absent for some reason

headObject(".quilt/named_packages/package_name") (just directory, without package revision) suddenly doesn't work

that's expected, bc head request only works for objects, and there's no such object, there's only prefix, so you should use listObjectsV2 for that prefix (you can limit number of keys to 1 to just see if there's anything under that prefix, no matter what)

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed requests.ensurePackageIsPresent. Now it uses request.bucketListing (which uses listObjectsV2). I just check if any file under .named_packages/package_name

@fiskus fiskus requested a review from nl0 January 14, 2021 11:21
@fiskus fiskus requested a review from nl0 January 14, 2021 18:37
@fiskus fiskus merged commit dd6ad48 into master Jan 15, 2021
@fiskus fiskus deleted the package-name-validation branch January 15, 2021 06:59
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.

3 participants