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.

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

Fix deprecated use of tar.TypeRegA (SA1019) #1980

Merged

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Jun 20, 2024

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

Partially fixes:

@Honny1 Honny1 force-pushed the fix-deprecated-tar_TypeRegA branch from 8ff1d25 to ef41778 Compare June 20, 2024 10:01
@Honny1 Honny1 marked this pull request as ready for review June 20, 2024 11:16
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

It’s not immediately obvious to me that we can just stop processing these values.

Is there a reason why this is safe? E.g. maybe because the inputs are being created by the same process and we can see the code that generated it? Or can these values never show up nowadays?

If this is processing externally-processed tarballs built by other implementations, or 7-year-old images, and those could actually generate those values, we should not be breaking that.


(I’m not saying that this is the wrong approach; from a very quick skim, it might be the right one — but I assume you have checked, so it would be nice to have a written record of the situation.)

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 20, 2024

For the record, in both affected places, the headers come from tar.Reader.Next, and that function never produces TypeRegA.

/lgtm

@Honny1
Copy link
Member Author

Honny1 commented Jun 20, 2024

I have the same thoughts. I checked the archive/tar library implementation for Go Lang version 1.21+. If the old version of the tarball header in the Typeflag contains the value TypeRegA, it is changed to TypeReg or TypeDir. Code reference. I think removing TypeRegA from the switch case and map shouldn't break other things. But I just did a static check. Any suggestions on how to check this more deeply and not only tests?

@Honny1 Honny1 force-pushed the fix-deprecated-tar_TypeRegA branch from ef41778 to ee13fd9 Compare June 20, 2024 15:44
@openshift-ci openshift-ci bot removed the lgtm label Jun 20, 2024
@mtrmac
Copy link
Collaborator

mtrmac commented Jun 20, 2024

In principle, generate a tar file like that, and see that it processed correctly.

golang/go@e4bde05 did add a test for the read code; we don’t have a test that we are using that source of the data.

On balance, I think adding tests for this situation is overkill. It wouldn’t hurt but there are whole subpackages in this project with no unit tests at all, that would be a better use of time.

(Alternatively, we could just silence the warning, but when the raw 0 byte can mean either a file and a directory, that would still be making an assumption that archive/tar.Reader is being used to convert the value — so we might just as well remove the cases and follow the documented API.)

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 20, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Jun 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Jun 20, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 20, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 11705ca into containers:main Jun 20, 2024
18 checks passed
@Honny1 Honny1 deleted the fix-deprecated-tar_TypeRegA branch July 1, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants