8000 Return ErrNotImplemented from unixgramListener on windows and linux by vyasgun · Pull Request #4765 · crc-org/crc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Return ErrNotImplemented from unixgramListener on windows and linux #4765

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
Merged
merged 1 commit into from
May 21, 2025
Merged

Return ErrNotImplemented from unixgramListener on windows and linux #4765

merged 1 commit into from
May 21, 2025

Conversation

vyasgun
Copy link
Contributor
@vyasgun vyasgun commented May 20, 2025 •
edited
Loading

Description

Error should not be nil in the case of unixgramListener implementation in daemon_linux.go and daemon_windows.go

Relates to: #4764, PR #4753

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Proposed changes

Testing

Contribution Checklist

  • I Keep It Small and Simple: The smaller the PR is, the easier it is to review and have it merged
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Which platform have you tested the code changes on?
    • Linux
    • Windows
    • MacOS

Summary by Sourcery

Return ErrNotImplemented for unsupported unixgramListener implementations on Linux and Windows and adjust macOS logging formatting

Bug Fixes:

  • Make unixgramListener on Linux return drivers.ErrNotImplemented instead of nil
  • Make unixgramListener on Windows return drivers.ErrNotImplemented instead of nil

Enhancements:

  • Remove extraneous colon in the logging message for the darwin unixgramListener

@sourcery-ai Sourcery AI
Copy link
sourcery-ai bot commented May 20, 2025 •
edited
Loading

Reviewer's Guide

This PR updates the unixgramListener implementation on Linux and Windows to return ErrNotImplemented (introducing the drivers import) and refines the Unixgram socket logging message on macOS by removing an extraneous colon.

Sequence Diagram: Updated unixgramListener Behavior on Linux/Windows

sequenceDiagram
    participant C as Caller
    participant UL as "unixgramListener (Linux/Windows)"
    participant D as "drivers package"

    C->>UL: Call unixgramListener(context, virtualnetwork)
    UL->>D: Access ErrNotImplemented
    UL-->>C: Return (nil, ErrNotImplemented)
Loading

File-Level Changes

Change Details Files
Return ErrNotImplemented in unixgramListener on Linux and Windows
  • Added import for drivers package
  • Changed return value from (nil, nil) to (nil, drivers.ErrNotImplemented)
 cmd/crc/cmd/daemon_linux.go
cmd/crc/cmd/daemon_windows.go
Refine logging message in Darwin unixgramListener
  • Removed trailing colon from log.Infof format string
 cmd/crc/cmd/daemon_darwin.go

Possibly linked issues

  • #0: The PR modifies the Windows daemon's unixgramListener to return an error instead of nil, likely fixing the panic in the issue.
Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@openshift-ci openshift-ci bot requested review from albfan and evidolob May 20, 2025 07:43
sourcery-ai[bot]
sourcery-ai bot reviewed May 20, 2025
View reviewed changes
Copy link
@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @vyasgun - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Documentation: all looks good
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@gbraad gbraad linked an issue May 20, 2025 that may be closed by this pull request
[bug] crc setup fail on windows as crc daemon hits panic to invalid pointer reference #4764
Closed
@vyasgun vyasgun force-pushed the pr/unixgram-darwin-only branch from c6f0ba7 to ba31670 Compare May 20, 2025 08:52
@anjannath
Copy link
Member

tested on windows 11 and crc setup is working now with the changes from this PR

@vyasgun vyasgun force-pushed the pr/unixgram-darwin-only branch from ba31670 to 180611a Compare May 20, 2025 09:52
cfergeau
cfergeau reviewed May 20, 2025
View reviewed changes
@vyasgun vyasgun force-pushed the pr/unixgram-darwin-only branch 5 times, most recently from df87093 to 836947e Compare May 21, 2025 06:20
@vyasgun vyasgun force-pushed the pr/unixgram-darwin-only branch from 836947e to 5ebadf8 Compare May 21, 2025 06:26
@praveenkumar
Copy link
Member

/retest

@openshift-ci OpenShift CI
Copy link
openshift-ci bot commented May 21, 2025

@vyasgun: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/integration-crc 5ebadf8 link true /test integration-crc
ci/prow/e2e-microshift-crc 5ebadf8 link true /test e2e-microshift-crc
ci/prow/images 5ebadf8 link true /test images
ci/prow/security 5ebadf8 link false /test security
ci/prow/e2e-crc 5ebadf8 link true /test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@vyasgun vyasgun requested a review from cfergeau May 21, 2025 07:55
cfergeau
cfergeau approved these changes May 21, 2025
View reviewed changes
Copy link
Contributor
@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label May 21, 2025
@openshift-ci OpenShift CI
Copy link
openshift-ci bot commented May 21, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

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

@praveenkumar praveenkumar merged commit d4637ce into crc-org:main May 21, 2025
20 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@cfergeau cfergeau cfergeau approved these changes

@albfan albfan Awaiting requested review from albfan

@evidolob evidolob Awaiting requested review from evidolob

Assignees

@cfergeau cfergeau

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] crc setup fail on windows as crc daemon hits panic to invalid pointer reference
4 participants
@vyasgun @anjannath @praveenkumar @cfergeau
0