8000 [release/1.2 backport] fix: SCHILY.xattrs should be SCHILY.xattr by thaJeztah · Pull Request #2953 · containerd/containerd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[release/1.2 backport] fix: SCHILY.xattrs should be SCHILY.xattr #2953

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
merged 1 commit into from
Jan 23, 2019
Merged

[release/1.2 backport] fix: SCHILY.xattrs should be SCHILY.xattr #2953

merged 1 commit into from
Jan 23, 2019

Conversation

thaJeztah
Copy link
Member
@thaJeztah thaJeztah commented Jan 23, 2019 •
edited
Loading

backport of #2873 for 1.2. Relates to #2942

from golang code
https://github.com/golang/go/blob/bad6b6fa9190e9079a6d6958859856a66f0fab87/src/archive/tar/common.go#L110

add unit test for tar xattr

Fixes: #2863

(linking for discoverability); Introduced in bc9cb25 (#1812)

@Ace-Tang @thaJeztah
fix: SCHILY.xattrs should be SCHILY.xattr
2244a20 
from golang code
https://github.com/golang/go/blob/bad6b6fa9190e9079a6d6958859856a66f0fab87/src/archive/tar/common.go#L110

add unit test for tar xattr

Fixes: containerd#2863

Signed-off-by: Ace-Tang <aceapril@126.com>
(cherry picked from commit 6f944e4)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

ping @estesp @kfox1111

@estesp
Copy link
Member
estesp commented Jan 23, 2019 •
edited
Loading

wow, nice catch @thaJeztah; should go in [release/1.1] as well?

edited

@thaJeztah
Copy link
Member Author
thaJeztah commented Jan 23, 2019 •
edited
Loading

@estesp perhaps yes, but I noticed that it doesn't apply cleanly; likely because #2696 and #2458 are missing; I see you mentioned considering that first one to be back ported (#2696 (review)), but it will have to be modified (because I think the "import" part is missing in the 1.1 branch)

@codecov-io
Copy link

Codecov Report

Merging #2953 into release/1.2 will decrease coverage by 3.6%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           release/1.2   #2953      +/-   ##
==============================================
- Coverage        47.41%   43.8%   -3.61%     
==============================================
  Files               91     100       +9     
  Lines             8407   10720    +2313     
==============================================
+ Hits              3986    4696     +710     
- Misses            3698    5295    +1597     
- Partials           723     729       +6
Flag Coverage Δ
#linux 47.48% <ø> (+0.07%) ⬆️
#windows 40.89% <ø> (?)
Impacted Files Coverage Δ
archive/tar.go 43.79% <ø> (-6.21%) ⬇️
snapshots/native/native.go 43.3% <0%> (-10%) ⬇️
metadata/snapshot.go 45.8% <0%> (-8.96%) ⬇️
metadata/containers.go 47.97% <0%> (-6.62%) ⬇️
content/local/writer.go 57.84% <0%> (-6.36%) ⬇️
content/local/store.go 48.51% <0%> (-5.03%) ⬇️
remotes/docker/resolver.go 58.36% <0%> (-4.99%) ⬇️
archive/tar_opts.go 28.57% <0%> (-4.77%) ⬇️
archive/compression/compression.go 58.69% <0%> (-4.7%) ⬇️
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c9ede4...2244a20. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #2953 into release/1.2 will decrease coverage by 3.6%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           release/1.2   #2953      +/-   ##
==============================================
- Coverage        47.41%   43.8%   -3.61%     
==============================================
  Files               91     100       +9     
  Lines             8407   10720    +2313     
==============================================
+ Hits              3986    4696     +710     
- Misses            3698    5295    +1597     
- Partials           723     729       +6
Flag Coverage Δ
#linux 47.48% <ø> (+0.07%) ⬆️
#windows 40.89% <ø> (?)
Impacted Files Coverage Δ
archive/tar.go 43.79% <ø> (-6.21%) ⬇️
snapshots/native/native.go 43.3% <0%> (-10%) ⬇️
metadata/snapshot.go 45.8% <0%> (-8.96%) ⬇️
metadata/containers.go 47.97% <0%> (-6.62%) ⬇️
content/local/writer.go 57.84% <0%> (-6.36%) ⬇️
content/local/store.go 48.51% <0%> (-5.03%) ⬇️
remotes/docker/resolver.go 58.36% <0%> (-4.99%) ⬇️
archive/tar_opts.go 28.57% <0%> (-4.77%) ⬇️
archive/compression/compression.go 58.69% <0%> (-4.7%) ⬇️
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c9ede4...2244a20. Read the comment docs.

@crosbymichael
Copy link
Member

LGTM

estesp
estesp approved these changes Jan 23, 2019
View reviewed changes
Copy link
Member
@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit fa60b5b into containerd:release/1.2 Jan 23, 2019
@thaJeztah thaJeztah deleted the 1.2_backport_fix_xattr branch January 23, 2019 21:41
@thaJeztah
Copy link
Member Author

Conflicts didn't look to bad; I went ahead and fixed those for the 1.1 branch (#2954), leaving the decision for #2696 and #2458 for later

@kfox1111 kfox1111 mentioned this pull request Feb 5, 2019
Merged
@ijc ijc mentioned this pull request Feb 11, 2019
Closed
This was referenced Feb 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estesp estesp estesp 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.
5 participants
@thaJeztah @estesp @codecov-io @crosbymichael @Ace-Tang
0