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 Lint on Windows and enable the job #21747

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

mheon
Copy link
Member

@mheon mheon commented Feb 19, 2024

Fix lint on Windows.

This may not pass CI initially, I don't know if we can make this work as cross-compile; we need CGo.

Does this PR introduce a user-facing change?

NONE

Copy link
Contributor

openshift-ci bot commented Feb 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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 Feb 19, 2024
@mheon
Copy link
Member Author

mheon commented Feb 19, 2024

Yeah, I don't know if this is going to work as a cross-build

Comment on lines 92 to 93
fmt.Fprintf(os.Stderr, "Error configuring logging: %v", err)
// Don't exit, it's just a logging issue
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we want to actually log this, #21426 seems to suggest that there are people running without event logger. cc @n1hility

Copy link
Member

Choose a reason for hiding this comment

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

yeah where this is ran its not actually visible (no console attached when ran from the installer), so it would only be visible for manual execution. If it were me I would just toss the error.

Re the issue. It's pretty atypical for Windows Logging to be not running since its an essential service (granted we shouldn't fail if its not running for exceptional reasons)

@n1hility
Copy link
Member

Yeah, I don't know if this is going to work as a cross-build

In this particular case we don't need CGO. (Golang builds for windows embed a special cross compile facility that is portable). There are only a few special cases where you need it (building DLLs, or using it it compile inline c code).

@n1hility
Copy link
Member

Looks like the hack/golangci-lint.sh needs to be updated to mirror the podman-remote task a little closer:

  • Adding the tag containers_image_openpgp to solve the gpgme error
  • Exlucding pkg/systemd for remote lint runs

@@ -40,10 +40,12 @@ func (e *ExitCodeError) Error() string {
return fmt.Sprintf("Process failed with exit code: %d", e.code)
}

//nolint:unused
Copy link
Member Author

Choose a reason for hiding this comment

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

@n1hility Can you take a look at this file and tell me if any of these are safe to remove? I kept them around assuming they might be used later.

Copy link
Member

Choose a reason for hiding this comment

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

some of these will be needed for the port cleanup logic, so if you could leave them around a bit. I can clean them up as part of that work. thanks!

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@mheon mheon force-pushed the windows_lint branch 6 times, most recently from 796909d to 10a7bd8 Compare February 19, 2024 22:49
[NO NEW TESTS NEEDED] Purely refactoring

Signed-off-by: Matt Heon <[email protected]>
@mheon
Copy link
Member Author

mheon commented Feb 20, 2024

podman search is flaking

@mheon
Copy link
Member Author

mheon commented Feb 20, 2024

Windows cross passed, which includes lint, so this is ready now

@baude baude added the 5.0 label Feb 20, 2024
@mheon
Copy link
Member Author

mheon commented Feb 20, 2024

Fourth rerun of HyperV tests. Different failures last two times. Timeouts on Podman commands, but different ones both times.

@rhatdan
Copy link
Member

rhatdan commented Feb 21, 2024

LGTM

@n1hility
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit f756e5d into containers:main Feb 21, 2024
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 22, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
5.0 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. machine release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants