search

containers / storage

Container Storage Library
Apache License 2.0
537 stars 234 forks source link

Fix deprecated use of `hdr.Xattrs` (SA1019) #1985

Closed Honny1 closed 3 days ago

Honny1 commented 6 days ago

This PR fixes warning deprecated use of hdr.Xattrs (SA1019) found by golangci when the staticcheck linter is enabled.

Partially fixes:

rhatdan commented 6 days ago

/approve LGTM @giuseppe @nalind @mtrmac @saschagrunert PTAL

openshift-ci[bot] commented 6 days ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/containers/storage/blob/main/OWNERS)~~ [rhatdan] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
rhatdan commented 6 days ago

@mtrmac PTANL

mtrmac commented 6 days ago

I’m fine with the code, but I’d like to hear an assurance that this is covered by tests. (Yes, I could go verify that myself…)

Honny1 commented 6 days ago

I checked the unit tests by commenting out the parts of the code that were changed if the tests fail. The tests did not fail for the ~readUserXattrToTarHeader~, (o overlayWhiteoutConverter) ConvertWrite, writeZstdChunkedStream functions. I haven't tried code changes that are specific to the darwin os. Should I also try to run unit tests of padman with the imported local storage?

mtrmac commented 6 days ago

I’d guess that if c/storage does not test these somewhat obscure aspects of layer extraction itself, Podman is unlikely to; but triggering a Podman CI run with a modified c/storage is probably much cheaper than manual analysis, so, sure, go for it.

Honny1 commented 5 days ago

@mtrmac I ran podman CI with imported c/storage with changes in this PR. CI passes see the PR that runs the tests.

Honny1 commented 5 days ago

I checked the unit tests by commenting out the parts of the code that were changed if the tests fail. The tests did not fail for the readUserXattrToTarHeader, (o overlayWhiteoutConverter) ConvertWrite, writeZstdChunkedStream functions. I haven't tried code changes that are specific to the darwin os. Should I also try to run unit tests of padman with the imported local storage?

I added an xattr to TestTarUntarWithXattr which is a user xattr. So the test fails for the commented change in readUserXattrToTarHeader.

mtrmac commented 4 days ago

/lgtm

mtrmac commented 4 days ago

@Honny1 the tests are failing

Honny1 commented 3 days ago

@mtrmac The tests are passing.

mtrmac commented 3 days ago

/lgtm