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

Document how delete_directory works on symlinks #7444

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Document how delete_directory works on symlinks #7444

merged 1 commit into from
Apr 9, 2024

Conversation

berland
Copy link
Contributor

@berland berland commented Mar 13, 2024 •
edited
Loading

Issue
Resolves user confusion in https://app.slack.com/client/T02JL00JU/CDPJULWR4

Approach
Test and document

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure tests pass locally (after every commit!)

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@berland berland self-assigned this Mar 13, 2024
@berland
Copy link
Contributor Author

berland commented Mar 13, 2024

@sondreso you might be opinionated here.

It is debatable whether we want to allow the trailing slashes in the argument to DELETE_DIRECTORY, one option would be to just error hard (breaking change!)

@berland berland added documentation release-notes:skip If there should be no mention of this in release notes labels Mar 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.21%. Comparing base (5d32021) to head (250f236).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7444      +/-   ##
==========================================
+ Coverage   85.20%   85.21%   +0.01%     
==========================================
  Files         387      387              
  Lines       23070    23070              
  Branches      889      875      -14     
==========================================
+ Hits        19656    19660       +4     
+ Misses       3307     3300       -7     
- Partials      107      110       +3

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

@berland
Copy link
Contributor Author

berland commented Mar 13, 2024

And the tests uncover that this is again behaves different on MacOS.. 🤯

@berland berland force-pushed the preciser_delete_dir branch 2 times, most recently from a115fc6 to 00c4d5b Compare March 14, 2024 12:48
@jonathan-eq
Copy link
Contributor

LGTM, but let's see what @sondreso says.

@sondreso you might be opinionated here.

It is debatable whether we want to allow the trailing slashes in the argument to DELETE_DIRECTORY, one option would be to just error hard (breaking change!)

@sondreso
Copy link
Collaborator

I think we should keep the default behavior here and not fail hard if there is a trailing slash. But the section in the documentation should be clearly marked (With a warning box) about the consequences of doing so.

@berland berland force-pushed the preciser_delete_dir branch 2 times, most recently from 66415f4 to 250f236 Compare March 22, 2024 12:48
@berland berland enabled auto-merge (rebase) March 22, 2024 13:04
@berland
Copy link
Contributor Author

berland commented Mar 22, 2024

Documentation now looks like:
image

xjules
xjules reviewed Apr 2, 2024
View reviewed changes
tests/unit_tests/shared/share/test_shell.py Outdated
def test_that_delete_directory_on_a_symlink_to_a_directory_is_conditionally_ignored(
shell, trailing
):
"""A trailing slash turns the world upside down"""
Copy link
Contributor

@xjules xjules Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

😆 the comment could be more descriptive - maybe detail down both scenarios

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Written more now

@berland berland force-pushed the preciser_delete_dir branch from 250f236 to 1bd1a90 Compare April 2, 2024 13:57
@berland berland force-pushed the preciser_delete_dir branch from 57c224a to 3ca7a65 Compare April 2, 2024 19:57
xjules
xjules approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@xjules xjules left a comment

Choose a reason for hiding this comment

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

Nice one! 🚀

@berland berland merged commit 48a15b7 into equinor:main Apr 9, 2024
37 checks passed
@eivindjahren eivindjahren mentioned this pull request Apr 25, 2024
Closed
@berland berland deleted the preciser_delete_dir branch May 29, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xjules xjules xjules approved these changes

Assignees

@berland berland

Labels
documentation release-notes:skip If there should be no mention of this in release notes
Projects
SCOUT
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@berland @codecov-commenter @jonathan-eq @sondreso @xjules