feat(snapshots): add symlink support for .kopiaignore #4190
Conversation
mcamou
changed the title
mcamou
force-pushed
the
add-ignore-symlink
branch
from
7a87d1b to
7a2c8d2
Compare
|
The force-push was after rebasing from |
|
@julio-lopez would you mind having a look at this? PRs have to be approved/merged in October. |
|
@julio-lopez sorry to be a pest, but I'd really like to have this PR count towards Hacktoberfest. |
|
@julio-lopez sorry to be such a pest, but the deadline for Hacktoberfest participation is tomorrow (the PR has to be approved by the end of October for it to count). |
mcamou
force-pushed
the
add-ignore-symlink
branch
2 times, most recently
from
ab1fb35 to
42612de
Compare
|
force-push after rebase from |
mcamou
force-pushed
the
add-ignore-symlink
branch
from
42612de to
5bc3450
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4190 +/- ##
==========================================
+ Coverage 75.86% 76.01% +0.14%
==========================================
Files 470 507 +37
Lines 37301 38946 +1645
==========================================
+ Hits 28299 29605 +1306
- Misses 7071 7378 +307
- Partials 1931 1963 +32 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
mcamou
force-pushed
the
add-ignore-symlink
branch
from
5bc3450 to
8ffd1ca
Compare
|
@julio-lopez Even though Hacktoberfest is over, I would really hate for this to go to waste. |
fs/localfs/local_fs_test.go
Outdated
| t.Errorf("entry is not a symlink: %s", path) | ||
| } else { | ||
| target, err := link.Resolve(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
please use: require.NoError(t, err) as is the convention
fs/ignorefs/ignorefs.go
Outdated
| return nil, errors.Wrapf(err, "when resolving symlink %s of type %T, which points to %s", entry.Name(), entry, link) | ||
| } | ||
|
|
||
| if f, ok := target.(fs.File); !ok { |
There was a problem hiding this comment.
what about symlink->symlink->file, would it make sense to resolve symlinks in a loop until we encounter a fs.File ?
There was a problem hiding this comment.
Probably a reasonable idea, I am not sure how necessary it might be especially in this specific case of ignorefs, but let's follow the principle of least surprise :)
|
sorry for late review, this looks very good, I left a couple comments, we can merge after those are addressed. please also rebase to the latest master (the UI test failures should be fixed there) |
|
@jkowalski Thanks for the review, I've addressed your comments. Please let me know if there's anything else. |
mcamou
force-pushed
the
add-ignore-symlink
branch
from
6d08da1 to
e55e357
Compare
mcamou
force-pushed
the
add-ignore-symlink
branch
from
e55e357 to
1df427c
Compare
jkowalski
changed the title
|
Thank you for the contribution! |
internal/mockfs/mockfs.go
Outdated
| "github.com/kopia/kopia/fs" | ||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
| "github.com/kopia/kopia/fs" | |
| "github.com/pkg/errors" | |
| "github.com/pkg/errors" | |
| "github.com/kopia/kopia/fs" |
| @@ -72,16 +72,19 @@ type Directory interface { | |||
| func IterateEntries(ctx context.Context, dir Directory, cb func(context.Context, Entry) error) error { | |||
| iter, err := dir.Iterate(ctx) | |||
| if err != nil { | |||
| return err //nolint:wrapcheck | |||
| return errors.Wrapf(err, "in fs.IterateEntries, creating iterator for directory %s", dir.Name()) | |||
There was a problem hiding this comment.
The pattern in the kopia code base (enforced by the linters) is to wrap errors coming from other packages.
What is the reason for wrapping the error returned by dir.Iterate() ?
There was a problem hiding this comment.
To give context regarding where the error came from (including the directory name)
julio-lopez
changed the title
No description provided.