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

Use local executor if hostAddr is local #205

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

koolay
Copy link

@koolay koolay commented May 18, 2024

No description provided.

@koolay koolay requested a review from umputun as a code owner May 18, 2024 02:33
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

thx for the PR. However, there are a few issues here:

  • Technically, the check you do for the localhost is way too broad. It will catch anything having "localhost" in it, like "localhost.mydomain.com".
    I don't think you should even handle 127.0.0.1 because it is at best a partial solution. Keeping just localhost as a special case would be better.
  • The change is lacking any corresponding tests.
  • The change is missing a readme/docs update.

In the discussion we had, I have emphasized why I do not like this approach, and you have not yet convinced me that this is even a good idea. However, if the final implementation will be as simple as it looks like now, I may approve it if the points above are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants