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.

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

[CI:BUILD] Cirrus: Catch use of deprecated ioutils package #15893

Merged
merged 1 commit into from
Sep 26, 2022
Merged

[CI:BUILD] Cirrus: Catch use of deprecated ioutils package #15893

merged 1 commit into from
Sep 26, 2022

Conversation

cevich
Copy link
Member

@cevich cevich commented Sep 21, 2022

At the time of this commit, there's no easier way to detect this using golangci-lint or the go tool (that I could find).

Signed-off-by: Chris Evich [email protected]

Does this PR introduce a user-facing change?

None

@cevich cevich requested a review from edsantiago September 21, 2022 19:12
@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 21, 2022
edsantiago
edsantiago suggested changes Sep 21, 2022
View reviewed changes
@edsantiago
Copy link
Member

Oh yeah. Can't actually submit any PRs until #15890 merges

@cevich cevich force-pushed the ioutil_whine branch 2 times, most recently from 4d3ecf1 to 84d6c08 Compare September 21, 2022 19:25
@cevich cevich changed the title [CI:BUILD] Cirrus: Catch use of deprecated ioutils package [WIP] [CI:BUILD] Cirrus: Catch use of deprecated ioutils package Sep 21, 2022
@cevich
Copy link
Member Author

cevich commented Sep 21, 2022

Oh yeah. Can't actually submit any PRs until #15890 merges

Yep 😞

cevich
cevich commented Sep 21, 2022
View reviewed changes
contrib/cirrus/runner.sh Outdated
regex=$(echo -e "^(\\+.+io/\x69outils)|(\\+.+\x69outils\\..+)")
if egrep -q "$regex"<<<"$diffs"; then
die "Found attempted use of deprecated ioutils:
$(egrep -B5 -A5 "$regex"<<<"$diffs")"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is imperfect, but simple. Hopefully 10-lines of context is enough for a PR author to find their problem.

Luap99
Luap99 reviewed Sep 22, 2022
View reviewed changes
@Luap99 Luap99 mentioned this pull request Sep 22, 2022
Merged
@cevich cevich force-pushed the ioutil_whine branch 2 times, most recently from 616fcd4 to 3aa04bf Compare September 22, 2022 15:33
@cevich
Copy link
Member Author

cevich commented Sep 22, 2022

Okay, this is working now as intended.

@cevich cevich marked this pull request as ready for review September 22, 2022 15:34
@cevich cevich changed the title [WIP] [CI:BUILD] Cirrus: Catch use of deprecated ioutils package [CI:BUILD] Cirrus: Catch use of deprecated ioutils package Sep 22, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2022
@cevich cevich requested review from edsantiago and Luap99 September 22, 2022 15:34
Luap99
Luap99 reviewed Sep 22, 2022
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@cevich
Copy link
Member Author

cevich commented Sep 22, 2022

Github is "having problems". I'll retry in a bit.

@Luap99
Copy link
Member

Luap99 commented Sep 22, 2022

No it must not, those bits are compiled also (golang 1.19 refuses to compile code that uses ioutil package).

I doubt that this is true. Golang has a compatibility promise and I highly doubt that they just broke that. Are you sure it is not just the linter complaining?

@cevich
Copy link
Member Author

cevich commented Sep 22, 2022

re you sure it is not just the linter complaining?

Oh it could be, you're probably right.

@edsantiago
Copy link
Member

Just dump out changed filenames from the diff. Loop over any matching *.go,

Can't: doesn't handle renames or removed files. (Without ugly muckery on the part of the script).

Here's how to do it:

git diff OLD NEW  -- '*.go' :^vendor/

Why didn't I go to stackoverflow in the first place? Why did I waste time (off and on, not all day, thankfully) reading git man pages?

@rhatdan
Copy link
Member

rhatdan commented Sep 23, 2022

While we are doing this we should also check for os.IsNotExists(err) versus the recommended errors.Is(err, os.ErrNotExist)

@cevich
Copy link
Member Author

cevich commented Sep 23, 2022

Why didn't I go to stackoverflow in the first place?

Yeah, I'm struggling with this too. StackOverflow first, then Google, then man 🤣

@cevich
Copy link
Member Author

cevich commented Sep 23, 2022

Can't: doesn't handle renames or removed files.

We probably don't care about removed files, but renames for sure. I'll try git diff OLD NEW -- '*.go' :^vendor/ but again, I'm worried about getting /something/ in place before more ioutil usage sneaks in and breaks my F37 PR. So I don't want to spend too long on it.

While we are doing this

Sure, but I would prefer to do that as a followup PR. Everybody and their uncle has trained-muscle-memory for typing ioutil... 😞

@cevich cevich mentioned this pull request Sep 23, 2022
Closed
@cevich
Cirrus: Catch use of deprecated io/ioutil package
69ac1e3 
At the time of this commit, there's no easier way to detect this using
`golangci-lint` or the go tool (that I could find).  A future update
to the `go list` command may support detection, for now use a CI script.

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Sep 23, 2022

Force-push: Rebased + implemented Ed's suggestion.

@cevich
Copy link
Member Author

cevich commented Sep 23, 2022

Okay, this seems to be working for a renamed file.

@cevich cevich mentioned this pull request Sep 23, 2022
Merged
@edsantiago
Copy link
Member

/lgtm
/hold

Thanks @cevich

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, edsantiago

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2022
@rhatdan
Copy link
Member

rhatdan commented Sep 26, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0a4a818 into containers:main Sep 26, 2022
cevich added a commit to cevich/podman that referenced this pull request Sep 26, 2022
@cevich
Cirrus: Add golang code consistency check script
527fc40 
Depends on containers#15893

Fixes: containers#15913

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich deleted the ioutil_whine branch April 18, 2023 14:48
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@edsantiago edsantiago edsantiago approved these changes

Assignees

@edsantiago edsantiago

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cevich @edsantiago @Luap99 @rhatdan @openshift-merge-robot