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

Fix #4206: allow root folders named '/' #4207

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

drernie
Copy link
Member

@drernie drernie commented Nov 5, 2024

Description

set_dir: Allow '/' folder in root.

@drernie drernie linked an issue Nov 5, 2024 that may be closed by this pull request
@drernie drernie changed the title 4206 allow root folders named 4206 allow root folders named '/' Nov 5, 2024
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.69%. Comparing base (5e7ce5d) to head (dcfb20f).

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #4207    +/-   ##
========================================
  Coverage   37.68%   37.69%            
========================================
  Files         768      768            
  Lines       35321    35326     +5     
  Branches     5021     5214   +193     
========================================
+ Hits        13312    13317     +5     
+ Misses      21402    20775   -627     
- Partials      607     1234   +627     
Flag Coverage Δ
api-python 91.09% <100.00%> (+<0.01%) ⬆️
catalog 12.07% <ø> (ø)
lambda 87.95% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drernie drernie self-assigned this Nov 5, 2024
@drernie
Copy link
Member Author

drernie commented Nov 5, 2024

We have a customer with a lot of data in a path like "s3://bucket//raw". I presume the check was in place to prevent accidentally entering an absolute path. Should we drop it entirely, or provide a warning?

@sir-sigurd sir-sigurd changed the title 4206 allow root folders named '/' Fix #4206: allow root folders named '/' Nov 6, 2024
Copy link
Member

@sir-sigurd sir-sigurd left a comment

Choose a reason for hiding this comment

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

generally seems right

needs tests

docs/CHANGELOG.md Outdated Show resolved Hide resolved
api/python/quilt3/util.py Outdated Show resolved Hide resolved
api/python/quilt3/util.py Outdated Show resolved Hide resolved
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 reasonable in general. i defer to @sir-sigurd on this one

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.

Allow root folders named '/'
3 participants