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

Run lint for mac builds #21372

Merged
merged 2 commits into from
Feb 3, 2024
Merged

Run lint for mac builds #21372

merged 2 commits into from
Feb 3, 2024

Conversation

cevich
Copy link
Member

@cevich cevich commented Jan 25, 2024 •
edited
Loading

Run golangci-lint for mac jobs but ignore all failures temporarily until they're fixed.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jan 25, 2024
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit d0f2d2a. @martinpitt, @jelly, @mvollmer please check.

@openshift-ci openshift-ci bot added release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jan 25, 2024
@martinpitt
Copy link
Contributor

Unrelated to this PR, but that's the second time I've seen a rawhide test run time out on podman build. This is a trivial Containerfile (just a FROM and a RUN), it really shouldn't take two minutes. That may be a regression in podman:next or the rawhide kernel, etc. Something to watch out for.

@cevich cevich force-pushed the win_mac_lint branch 2 times, most recently from bdce046 to 1ed777d Compare January 26, 2024 15:08
@cevich cevich changed the title [WIP] [CI:MACHINE] Run lint for mac builds Run lint for mac builds Jan 26, 2024
@cevich
Copy link
Member Author

cevich commented Jan 26, 2024

@martinpitt presumably you're talking about RuntimeError: Timed out on 'podman build -t localhost/test-intermediate /tmp/tmp.ABfI0DuVUL'? I don't know anything about these tests unfortunately. Is there someone you can recommend that should be altered to this?

@cevich
Copy link
Member Author

cevich commented Jan 26, 2024

@baude @edsantiago this is ready for an initial look. It's not perfect or pretty but should get the job done.

@cevich cevich marked this pull request as ready for review January 26, 2024 15:57
@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 Jan 26, 2024
edsantiago
edsantiago requested changes Jan 29, 2024
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Minor edits needed, and one question for the team.

hack/golangci-lint.sh Outdated Show resolved Hide resolved
hack/golangci-lint.sh Outdated Show resolved Hide resolved
hack/golangci-lint.sh Outdated Show resolved Hide resolved
hack/golangci-lint.sh Show resolved Hide resolved
@cevich
Copy link
Member Author

cevich commented Jan 29, 2024

Force-push: Addressed all/most of Eds concerns.

In case it gets lost, Ed's general question was:

[AT]containers/podman-maintainers, why is there no "remote" in the `for` loop? Does that have 
anything to do with the `TODO` on line 30?

FWIW I ran `remote` on my laptop and it passed.

@edsantiago
Copy link
Member

lint log. Will someone be working on those?

@cevich
Copy link
Member Author

cevich commented Jan 29, 2024

Will someone be working on those?

My understanding is yes, the intent is to get them fixed up at some point in the near(?) future. I can't say for certain whom is doing that. I got a "SGTM" from Brent Re: || true ignoring the results for now.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 29, 2024

[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 Jan 29, 2024
@cevich
Copy link
Member Author

cevich commented Jan 30, 2024

@baude PTAL, this is ready.

@mheon
Copy link
Member

mheon commented Jan 30, 2024

LGTM

@cevich
Run lint for mac builds
d9ad7cb 
There are darwin-specific code paths which were not being linted prior
to this commit.  Fix this with a new, darwin-specific section of the lint
runner script.

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich
Temporarily ignore mac-linting failures
3f3d1e4 
As of this commit, there are several pages worth of lint findings for
the mac.  Once they're all addressed, this commit may be reverted to
enable continuous checking.

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich
Copy link
Member Author

cevich commented Feb 1, 2024

force-push: rebased

@cevich cevich mentioned this pull request Feb 1, 2024
Merged
@cevich
Copy link
Member Author

cevich commented Feb 2, 2024

IMHO, this is ready to merge.

@rhatdan
Copy link
Member

rhatdan commented Feb 3, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit daf7a2c into containers:main Feb 3, 2024
91 of 92 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 6, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon mheon left review comments

@edsantiago edsantiago edsantiago approved these changes

@baude baude Awaiting requested review from baude

Assignees

@rhatdan rhatdan

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 @martinpitt @edsantiago @mheon @rhatdan