fileperms: newer Go 1.13+ octal literal format

[lsm5]

Problem: While removing cgroupsv1 code, I noticed my neovim Go config automatically changed fileperms to the new octal format and I didn't want that polluting my diffs.

Decision: I thought it best to switch to the new octal format in a dedicated PR.

Action:

• Cursor switched to new octal format for all fileperm ocurrences in Go source and test files.
• vendor/, docs/ and non-Go files were ignored.
• Reviewed manually.

Ref: https://go.dev/ref/spec#Go_1.13

Does this PR introduce a user-facing change?

None

(We can block this until Podman 6 if we're not comfortable with this going in earlier, but it'd be really nice to have it go in sooner (maybe even v5.7, assuming nothing breaks)).

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5

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 [lsm5]

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

[lsm5]

@containers/podman-maintainers PTAL

[TomSweeneyRedHat]

IDK, but there are so many instances of some of these values in a file. Would it make more sense to make them variables so that, in case this ever needs to change again, it's only in one spot per file?

[lsm5]

I'd be cool with that if everyone agrees.

[Luap99]

I don't see a strong benifit in doing this, the nice thing about having the modes inline any test reader sees which mode is being used with extra indirections.

I would say it would be much nicer to consolidate the common setup code into a helper function, i.e. create dir under the test tempdir and check for errors so in each test block we only have a single line. But of course that would be a much larger scope which I don't think we need to do here and all at once, we can introduce tests helpers gradually

[mtrmac]

In a lot of these situations, testing.T.Tempdir() is easier — no Join calls, no naming of the directory, just call Tempdir() and assign to a variable. Historically that was not possible in Ginkgo, reportedly it does work now. Or maybe we can build a utility like that.

Do we need to specify permissions for the directories? In many other projects, the default of 0700 is both safe and sufficient; it’s possible that Podman, with rootless operation, might need the permissions relaxed in some cases.

Clearly a different PR, though.

[Luap99]

Do we need to specify permissions for the directories? In many other projects, the default of 0700 is both safe and sufficient; it’s possible that Podman, with rootless operation, might need the permissions relaxed in some cases.

Yeah mode wise when using user namespaces and such we might need world readable when running the container as different uid, etc.. Depends a lot on the context of course.

And on the similar Tmpdir() will try to remove the dir via a normal remove() which is not enough if a file with subuids was created on it as part of a container run, see

podman/test/e2e/common_test.go

Lines 1310 to 1327 in 86eecc9

	// rmAll removes the directory and its content, when running rootless we use
	// podman unshare to prevent any subuid/gid problems
	func rmAll(podmanBin string, path string) {
	// Remove cache dirs
	if isRootless() {
	// If rootless, os.RemoveAll() is failed due to permission denied
	cmd := exec.Command(podmanBin, "unshare", "rm", "-rf", path)
	cmd.Stdout = GinkgoWriter
	cmd.Stderr = GinkgoWriter
	if err := cmd.Run(); err != nil {
	GinkgoWriter.Printf("%v\n", err)
	}
	} else {
	if err = os.RemoveAll(path); err != nil {
	GinkgoWriter.Printf("%q\n", err)
	}
	}
	}

As such I think is is somewhat important that we ensure the tmpfiles stay under the main podmanTest.Tempdir where possible so we can correctly ensure removal in all cases.

[TomSweeneyRedHat]

The machine-wsl test flaked, I've restarted.
LGTM
I've one thought for consideration, otherwise LGTM. I'd vote for getting this into v5.7 unless someone has strong concerns.

[Luap99]

While removing cgroupsv1 code, I noticed my neovim Go config automatically changed fileperms to the new octal format and I didn't want that polluting my diffs.

That more hints like you are using the wrong formatter then, the file mode may only be one example where it shows. I guess you are using gofumpt and not the normal gofmt?

That matters because gofumpt does much more changes and I really really hate these unrelated diff changes, I have been seeing them in other PRs at well. I think @ninja-quokka is on of the people where this is the same issue.

In general style like this must be enforced by the linter/formatters because not doing that will just result in new instances of 0755 that you then format in unrelated PRs most likely over time. And that is just not good.

So I see two ways, either we all agree that we want to format with gofumpt and add it to the linter list, we already have it on in some modules, compare containers/container-libs#352.

Or we keep doing the normal gofmt like it is and contributors must fix there editors to not format with tools like gofumpt and/or not format unrelated code.

I don't see a strong reason for gofumpt, the downside is that anybody not using it right now must fix their editors to add that into their local setup and for new contributors it creates another hurdle of failing the linter for some very nit picky style things. It also creates new friction on linter upgrades as it is possible that some new code must be reformatted, see the PR linked above.

@containers/podman-maintainers WDYT?

[Luap99]

I'd vote for getting this into v5.7 unless someone has strong concerns.

I don't see any reason for or against it, it can be merged anytime as it doesn't chnage anything code logic wise. There is no benifit over blocking the 5.7 release on it in other words if it isn't merged by then.

[Luap99]

I don't think the formatter would chnage this btw, this seems to be a comment in an xml file so it likely doesn't matter at all but it isn't really related to the go syntax here at all and using the more comment 0600 mode like you would do on command like chmod would thus make more sense in the context here I would argue.

[ninja-quokka]

+1

[lsm5]

ya, this change was made by cursor and evidently i ignored this in the manual review :|

[ninja-quokka]

So I see two ways, either we all agree that we want to format with gofumpt and add it to the linter list, we already have it on in some modules, compare containers/container-libs#352.

Or we keep doing the normal gofmt like it is and contributors must fix there editors to not format with tools like gofumpt and/or not format unrelated code.

I don't have a strong opinion either way. I was using gofumpt because I naively assumed "stricter" was better. I've now come to think in a large project like Podman with so many contributors the most important thing when it comes to linters is uniformity of use.

I think we should use gofmt unless someone has strong justification and makes the case for using gofumpt. At that point we should update our CI, docs, validatepr etc.

Having said that I like this current patch and think keeping the code base updated with changing code patterns/standards is a positive thing, I just share the concern with @Luap99 that without correct onboarding of gofumpt new patches will be introduced using the old style and thus undermining this refactoring effort.

[lsm5]

I'm ok either way. If we decide on gofumpt, we should enforce it in a CI check, like Lewis mentioned. If I have a vote, I'll say stricter is better, unless it's known to break things. But that could also mean some transition time where a few people might be occupied fixing gofumpt issues to make CI green with gofumpt enabled.

[lsm5]

Or we keep doing the normal gofmt like it is and contributors must fix there editors to not format with tools like gofumpt and/or not format unrelated code.

How about new contributors who are already using gofumpt elsewhere? Should they be asked to switch to gofmt and resubmit PRs? (If this is expected to be a rare case, then I guess we can ignore this.)

[Luap99]

Or we keep doing the normal gofmt like it is and contributors must fix there editors to not format with tools like gofumpt and/or not format unrelated code.

How about new contributors who are already using gofumpt elsewhere? Should they be asked to switch to gofmt and resubmit PRs? (If this is expected to be a rare case, then I guess we can ignore this.)

Well gofumpt is AFAIK a superset of the normal gofmt. So a submitter can have code formatter with gofumpt as long as it is only used on the newly written code and thus nor reformat the rest of the file. Given the formatters work file based that seems impossible which means reviews are harder than they should be in which case I am going to ask the submitter to remove the formatting changes please.

I have no real clue what the percentage of users is for each formatter. I am fine with adopting either one personally but I guess that is something that we should decide once with a majority agreement

Though I guess technically there is no requirement to block this PR on it. Though as I said before if there is nothing enforcing the style than this will not solve your problem of formatting unrelated code.

[lsm5]

Ack, I'd like to include this as a community cabal topic in that case. I'll fix the xml change in this PR and run another manual review. And for other stuff, I'll use gofmt until we reach a decision.

[Luap99]

re: linter discussion, please let's continue it here: #27291

[lsm5]

I've removed the xml and comment diffs from this PR. PTAL