Skip to content

feat(snapshots): add symlink support for .kopiaignore #4190

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
Merged
jkowalski merged 3 commits into from
Nov 19, 2024
Merged

feat(snapshots): add symlink support for .kopiaignore #4190

jkowalski merged 3 commits into from
Nov 19, 2024

Conversation

@mcamou
Copy link
Contributor

@mcamou mcamou commented Oct 21, 2024

No description provided.

@mcamou mcamou mentioned this pull request Oct 21, 2024
Closed
@mcamou mcamou changed the title [Fixes #2037] Add symlink support for .kopiaignore fix(infra): Fix for #2037 Add symlink support for .kopiaignore Oct 21, 2024
@mcamou mcamou marked this pull request as ready for review October 21, 2024 18:47
@mcamou mcamou force-pushed the add-ignore-symlink branch from 7a87d1b to 7a2c8d2 Compare October 22, 2024 08:48
@mcamou
Copy link
Contributor Author

mcamou commented Oct 22, 2024

The force-push was after rebasing from master.

@mcamou
Copy link
Contributor Author

mcamou commented Oct 25, 2024

@julio-lopez would you mind having a look at this? PRs have to be approved/merged in October.

@tecosaur tecosaur mentioned this pull request Oct 28, 2024
Open
@mcamou
Copy link
Contributor Author

mcamou commented Oct 28, 2024

@julio-lopez sorry to be a pest, but I'd really like to have this PR count towards Hacktoberfest.

@mcamou
Copy link
Contributor Author

mcamou commented Oct 30, 2024

@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 mcamou force-pushed the add-ignore-symlink branch 2 times, most recently from ab1fb35 to 42612de Compare October 30, 2024 13:44
@mcamou
Copy link
Contributor Author

mcamou commented Oct 30, 2024

force-push after rebase from master

@mcamou mcamou force-pushed the add-ignore-symlink branch from 42612de to 5bc3450 Compare October 30, 2024 18:26
@codecov
Copy link

codecov bot commented Nov 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 56.09756% with 18 lines in your changes missing coverage. Please review.

Project coverage is 76.01%. Comparing base (cb455c6) to head (1df427c).
Report is 356 commits behind head on master.

Files with missing lines Patch % Lines
fs/ignorefs/ignorefs.go 61.53% 8 Missing and 2 partials ⚠️
fs/entry.go 40.00% 2 Missing and 1 partial ⚠️
fs/localfs/local_fs.go 62.50% 2 Missing and 1 partial ⚠️
snapshot/snapshotfs/repofs.go 0.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@mcamou mcamou force-pushed the add-ignore-symlink branch from 5bc3450 to 8ffd1ca Compare November 5, 2024 12:41
@mcamou
Copy link
Contributor Author

mcamou commented Nov 11, 2024

@julio-lopez Even though Hacktoberfest is over, I would really hate for this to go to waste.

jkowalski
jkowalski reviewed Nov 16, 2024
View reviewed changes
jkowalski
jkowalski reviewed Nov 16, 2024
View reviewed changes
@jkowalski
Copy link
Contributor

jkowalski commented Nov 16, 2024

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)

@mcamou
Copy link
Contributor Author

mcamou commented Nov 18, 2024

@jkowalski Thanks for the review, I've addressed your comments. Please let me know if there's anything else.

@mcamou mcamou force-pushed the add-ignore-symlink branch from 6d08da1 to e55e357 Compare November 18, 2024 16:25
@mcamou mcamou force-pushed the add-ignore-symlink branch from e55e357 to 1df427c Compare November 18, 2024 16:29
@jkowalski jkowalski changed the title fix(infra): Fix for #2037 Add symlink support for .kopiaignore feat(snapshots): Fix for #2037 Add symlink support for .kopiaignore Nov 19, 2024
@jkowalski jkowalski enabled auto-merge (squash) November 19, 2024 06:35
@jkowalski
Copy link
Contributor

jkowalski commented Nov 19, 2024

Thank you for the contribution!

@jkowalski jkowalski merged commit 5ce6b8d into kopia:master Nov 19, 2024
julio-lopez
julio-lopez reviewed Jan 10, 2025
View reviewed changes
Copy link
Collaborator

@julio-lopez julio-lopez left a comment

Choose a reason for hiding this comment

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

@mcamou thanks for your contribution.

See inline comments

internal/mockfs/mockfs.go
"github.com/kopia/kopia/fs"
"github.com/pkg/errors"
Copy link
Collaborator

@julio-lopez julio-lopez Oct 25, 2024

Choose a reason for hiding this comment

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

Suggested change
"github.com/kopia/kopia/fs"
"github.com/pkg/errors"
"github.com/pkg/errors"
"github.com/kopia/kopia/fs"

fs/entry.go
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())
Copy link
Collaborator

@julio-lopez julio-lopez Oct 25, 2024

Choose a reason for hiding this comment

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

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() ?

Copy link
Contributor Author

@mcamou mcamou Jan 10, 2025

Choose a reason for hiding this comment

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

To give context regarding where the error came from (including the directory name)

@julio-lopez julio-lopez mentioned this pull request Jan 18, 2025
Merged
julio-lopez added a commit that referenced this pull request Jan 18, 2025
@julio-lopez
test(general): cleanup symlink tests (#4347)
f382a99 
Ref: feat(snapshots): Fix for #2037 Add symlink support for .kopiaignore #4190
@julio-lopez julio-lopez changed the title feat(snapshots): Fix for #2037 Add symlink support for .kopiaignore feat(snapshots): add symlink support for .kopiaignore Feb 15, 2025
@julio-lopez
Copy link
Collaborator

julio-lopez commented Feb 15, 2025

@julio-lopez julio-lopez mentioned this pull request Feb 15, 2025
Merged
julio-lopez added a commit that referenced this pull request Feb 15, 2025
@julio-lopez
fix(general): prevent infinite loop while resolving ignore file symli…
a394a50 
…nks (#4413)

* test symlink infinite loop
* remove spurious error wrap
* cleanup error messages
* remove unneeded intermediate vars
* check error in loop in fs.GetAllEntries
  While cur is expected to be `nil` when there's an error, this
  makes the check explicit.

Ref:
- #2037
- #4190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julio-lopez julio-lopez julio-lopez left review comments

@jkowalski jkowalski jkowalski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mcamou @jkowalski @julio-lopez