Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

v0.8.3 bug fix: ping endpoint #59

Merged
merged 16 commits into from
Aug 4, 2021
Merged

v0.8.3 bug fix: ping endpoint #59

merged 16 commits into from
Aug 4, 2021

Conversation

AIAmber
Copy link
Member

@AIAmber AIAmber commented Aug 3, 2021

v0.8.3 bug fix: ping endpoint

1.get host from endpoint
2.ping host
3.pylint

AIAmber added 4 commits August 3, 2021 10:39
1.get host from endpoint
2.ping host
v0.8.3 bug fix: ping endpoint
1.get host from endpoint
2.ping host
@AIAmber AIAmber requested a review from a team as a code owner August 3, 2021 02:49
Copy link
Collaborator

@wj-Mcat wj-Mcat left a comment

Choose a reason for hiding this comment

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

Thanks for your great issue to solve ping problem with endpoint, Please refer to this issue: wechaty/python-wechaty#233

@@ -898,7 +898,12 @@ def _init_puppet(self):
log.debug('get puppet ip address : <%s>', data)
self.options.end_point = '{ip}:{port}'.format(**data)

if ping(self.options.end_point) is False:
if ':' not in self.options.end_point:
Copy link
Collaborator

Choose a reason for hiding this comment

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

: is not required for the valid endpoint which can be the service name or url. Eg: wechaty-service in docker network or http://api.wechaty-service.com .

Copy link
Member Author

Choose a reason for hiding this comment

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

i got it

Copy link
Member Author

@AIAmber AIAmber Aug 3, 2021

Choose a reason for hiding this comment

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

we can make a rule:

WECHATY_PUPPET_SERVICE_ENDPOINT must be written as {Host/Domain Name}:{port}

if use default port, we must write it. Eg: for http, we use 80, for https, we use 443

Copy link
Member Author

Choose a reason for hiding this comment

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

we must follow the identical rules when we write the WECHATY_PUPPET_SERVICE_ENDPOINT, otherwise there will be many cases that we can't deal with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe there must be some bugs in ping code. but port is not required in ping function. Can you reproduce your problem? And can you fix it after add port support ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you mean that it needs telnet.

socket is a good way, now i'm looking for the methods about socket

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, looking forward for your latest commit. You can encapsulate it into utils.py module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are some developers waiting for this pr, please complete the development and testing of this feature as soon as possible. Many thanks for your sharing and understanding.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, i will make it soon

Copy link
Member Author

Choose a reason for hiding this comment

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

I did a simple local test, no problem. please review my code.

AIAmber added 2 commits August 4, 2021 10:48
1.ping port
2.if not port, ping host
v0.8.3 bug fix2: test endpoint
Copy link
Collaborator

@wj-Mcat wj-Mcat left a comment

Choose a reason for hiding this comment

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

Please fix linting to keep the code quality. Thanks please.

Comment on lines 47 to 62
def test_endpoint(end_point: str) -> float:
"""
test_endpoint:
1.If there is port: telnet
2.If there is host/domain: ping or
"""
tn = Telnet()
host, port = extract_host_and_port(end_point)
if port is None:
return ping(host)
else:
try:
tn.open(host, port=port)
except Exception:
return False
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your return data type is Wrong:
You define the return type is float, but you return float/bool data type. This code will not pass CI testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

no, there are 2 pylint errors:
1./puppet.py:37:0: W0611: Unused ping imported from ping3 (unused-import)
2./utils.py:55:4: R1705: Unnecessary "else" after "return" (no-else-return)

Copy link
Member Author

Choose a reason for hiding this comment

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

i just fix these pylint errors, let it run CI test.

Copy link
Member Author

@AIAmber AIAmber left a comment

Choose a reason for hiding this comment

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

1

@AIAmber AIAmber requested a review from wj-Mcat August 4, 2021 03:10
@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

Please fix linting to keep the code quality. Thanks please.

1

@wj-Mcat
Copy link
Collaborator

wj-Mcat commented Aug 4, 2021

Please fix linting to keep the code quality. Thanks please.

1

??? Your CI testing is down.

@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

Please fix linting to keep the code quality. Thanks please.

1

??? Your CI testing is down.

oh no, there are 2 other pylint errors:
1./utils.py:63:15: W0703: Catching too general exception Exception (broad-except)
2./utils.py:25:0: C0411: standard import "from telnetlib import Telnet" should be placed before "from wechaty_puppet.exceptions import WechatyPuppetConfigurationError" (wrong-import-order)

@wj-Mcat
Copy link
Collaborator

wj-Mcat commented Aug 4, 2021

Please fix linting to keep the code quality. Thanks please.

1

??? Your CI testing is down.

oh no, there are 2 other pylint errors:
1./utils.py:63:15: W0703: Catching too general exception Exception (broad-except)
2./utils.py:25:0: C0411: standard import "from telnetlib import Telnet" should be placed before "from wechaty_puppet.exceptions import WechatyPuppetConfigurationError" (wrong-import-order)

This is the general linting error, I believe you can fix it. Go ahead.

@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

Please fix linting to keep the code quality. Thanks please.

1

??? Your CI testing is down.

oh no, there are 2 other pylint errors:
1./utils.py:63:15: W0703: Catching too general exception Exception (broad-except)
2./utils.py:25:0: C0411: standard import "from telnetlib import Telnet" should be placed before "from wechaty_puppet.exceptions import WechatyPuppetConfigurationError" (wrong-import-order)

This is the general linting error, I believe you can fix it. Go ahead.

Yes, I will try again.

@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

Please fix linting to keep the code quality. Thanks please.

1

??? Your CI testing is down.

oh no, there are 2 other pylint errors:
1./utils.py:63:15: W0703: Catching too general exception Exception (broad-except)
2./utils.py:25:0: C0411: standard import "from telnetlib import Telnet" should be placed before "from wechaty_puppet.exceptions import WechatyPuppetConfigurationError" (wrong-import-order)

This is the general linting error, I believe you can fix it. Go ahead.

Yes, I will try again.
Could you recommend a good pylint tool for me to do some local testing?

@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

i try it again, then i will install pylint and do some local testing.

i'm sorry to have incommoded you for my pylint

@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

yes, pylint passed!

i add ping # type ignore, please approve it again

ping type ignore
@wj-Mcat
Copy link
Collaborator

wj-Mcat commented Aug 4, 2021

Awesome, the last step: bump the version in VERSION file to trigger the CD stage.

Copy link
Collaborator

@wj-Mcat wj-Mcat left a comment

Choose a reason for hiding this comment

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

LGTM

@wj-Mcat
Copy link
Collaborator

wj-Mcat commented Aug 4, 2021

Link to wechaty/PMC#16

@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

Link to wechaty/PMC#16

yeah, i make it done.

last,
i got a lot about this project, and THANK YOU a lot.

@wj-Mcat wj-Mcat merged commit d1c0c38 into wechaty:master Aug 4, 2021
@wj-Mcat
Copy link
Collaborator

wj-Mcat commented Aug 4, 2021

If you wish, you can continue to participate in the python-wechaty project development, issue processing and documentation improvements. Welcome to become contrbutor of python-wechaty 🎉 🎉 🎉

@AIAmber
Copy link
Member Author

AIAmber commented Aug 4, 2021

Yes, I think I will do it better

@huan
Copy link
Member

huan commented Aug 4, 2021

Thank you very much for your contribution!

You are welcome to join Wechaty Contributor Program

1. Join Wechaty Organization

You've invited AIAmber to Wechaty! They'll be receiving an email shortly. They can also visit https://github.com/wechaty to accept the invitation.

I have invited you to join our Wechaty GitHub Organization, please accept it by following the above message. (See also: wechaty/PMC#16)

2. Update Your Wechaty Contributor Profile

  1. Please open Contributor Hall of Fame and add yourself to the end of the list, so that other contributors will know you better!
  2. Please add yourself to our Website Contributors by creating a PR and refer to this PR link as well.

3. Join The Contributor Only WeChat Room

We also have a WeChat room for contributors only which can discuss Wechaty at a deeper level, you are welcome to join and if you are interested.

Please add @lijiarui wechat: ruirui_0914 and send her this pr link. She will invite you into Wechaty Contributor Room

Cheers!

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

Successfully merging this pull request may close these issues.

3 participants