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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 30 commits into from
Nov 21, 2024
Merged

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

merged 30 commits into from
Nov 21, 2024

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
Allow root folders named '/' #4206
Closed
@drernie drernie changed the title 4206 allow root folders named 4206 allow root folders named '/' Nov 5, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.16%. Comparing base (7fe641e) to head (424a746).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4207      +/-   ##
==========================================
+ Coverage   39.14%   39.16%   +0.02%     
==========================================
  Files         775      775              
  Lines       35072    35086      +14     
  Branches     5294     5093     -201     
==========================================
+ Hits        13729    13743      +14     
- Misses      20146    20777     +631     
+ Partials     1197      566     -631
Flag Coverage Δ
api-python 91.10% <100.00%> (+0.01%) ⬆️
catalog 14.57% <ø> (ø)
lambda 92.02% <ø> (ø)

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.

🚨 Try these New Features:

@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
sir-sigurd
sir-sigurd reviewed Nov 6, 2024
View reviewed changes
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
nl0
nl0 reviewed Nov 6, 2024
View reviewed changes
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

@drernie drernie requested a review from Copilot November 17, 2024 05:33
Copilot
Copilot AI reviewed Nov 17, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.
sir-sigurd
sir-sigurd reviewed Nov 19, 2024
View reviewed changes
api/python/tests/integration/test_packages.py Outdated Show resolved Hide resolved
api/python/quilt3/data_transfer.py Outdated Show resolved Hide resolved
sir-sigurd
sir-sigurd reviewed Nov 20, 2024
View reviewed changes
api/python/tests/integration/test_packages.py Outdated Show resolved Hide resolved
api/python/tests/integration/test_packages.py Outdated Show resolved Hide resolved
docs/CHANGELOG.md Outdated Show resolved Hide resolved
@sir-sigurd sir-sigurd requested a review from nl0 November 21, 2024 10:11
nl0
nl0 approved these changes Nov 21, 2024
View reviewed changes
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.

lgtm

@sir-sigurd sir-sigurd added this pull request to the merge queue Nov 21, 2024
Merged via the queue into master with commit 4d8d562 Nov 21, 2024
38 checks passed
@sir-sigurd sir-sigurd deleted the 4206-allow-root-folders-named branch November 21, 2024 11:15
nl0 added a commit that referenced this pull request Nov 21, 2024
@nl0
Merge branch 'master' into uri-catalog
6d08e54 
* master:
  Prettify `Overview.tsx` (#4233)
  Fix #4206: allow root folders named '/' (#4207)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@sir-sigurd sir-sigurd sir-sigurd left review comments

@nl0 nl0 nl0 approved these changes

Assignees

@drernie drernie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow root folders named '/'
3 participants
@drernie @nl0 @sir-sigurd