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

[Feature] Adds a transport class for the Http Input Plugin #51

Closed
wants to merge 28 commits into from

Conversation

jloehel
Copy link
Contributor

@jloehel jloehel commented Jul 1, 2020

This commit adds a more generic class called Transport and a additional
class HttpTransport to create a client for the Http Input Plugin of
logstash: https://www.elastic.co/guide/en/logstash/current/plugins-inputs-http.html#plugins-inputs-http

Signed-off-by: Jürgen Löhel [email protected]

@jloehel jloehel force-pushed the feature/transport_http branch 4 times, most recently from 5617d7e to bb07fb8 Compare July 2, 2020 16:52
@jloehel jloehel marked this pull request as draft July 3, 2020 03:11
@jloehel jloehel force-pushed the feature/transport_http branch 2 times, most recently from d0522d8 to 3a37570 Compare July 3, 2020 20:17
Copy link
Owner

@eht16 eht16 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 contribution.
I added a few remarks and questions, it would be nice to get them resolved.

Generally, it would be cool to follow the existing code style like defining all instance attributes in __init__(), use single quotes for string literals.
Also updating the documentation about the new transport is missing.

.travis.yml Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Show resolved Hide resolved
@jloehel
Copy link
Contributor Author

jloehel commented Jul 5, 2020

Thanks for contribution.
I added a few remarks and questions, it would be nice to get them resolved.

Thank you very much for your review. I will answer your question and I will add the documentation as quickly as possible.

edit: Usually I use now the black code style. But I will convert all the double quotes back into single quotes.

@jloehel
Copy link
Contributor Author

jloehel commented Jul 7, 2020

@eht16 I changed the code. Can you please review it again.

@jloehel jloehel marked this pull request as ready for review July 7, 2020 16:59
@jloehel
Copy link
Contributor Author

jloehel commented Jul 7, 2020

They released isort 5.0.4 yesterday and removed SortImports. But pylint is still using it.

@eht16
Copy link
Owner

eht16 commented Jul 7, 2020

They released isort 5.0.4 yesterday and removed SortImports. But pylint is still using it.

I know. There is pylint-dev/pylint#3722 and pylint-dev/pylint#3725.
I hope a compatible Pylint version will be released soon.

@eht16
Copy link
Owner

eht16 commented Jul 7, 2020

edit: Usually I use now the black code style. But I will convert all the double quotes back into single quotes.

That's one of the reasons I don't use black: they force people to use their style. I got the main idea of establishing a common code style and that they think double quotes are better or whatever but I'd like to be able to choose. More generally, the lack of configuration options by design in black makes it unusable for me.
So I tend to stick with isort, flake8 and pylint.

logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
Copy link
Owner

@eht16 eht16 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 changes. Much appreciated.
I got only a few more minor remarks.

logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
@jloehel jloehel closed this Jul 7, 2020
@jloehel
Copy link
Contributor Author

jloehel commented Jul 7, 2020

Sorry, I closed it accidentally ^^

@jloehel jloehel reopened this Jul 7, 2020
@eht16
Copy link
Owner

eht16 commented Jul 8, 2020

They released isort 5.0.4 yesterday and removed SortImports. But pylint is still using it.

I know. There is PyCQA/pylint#3722 and PyCQA/pylint#3725.
I hope a compatible Pylint version will be released soon.

If you want, you can apply the following changes to tox.ini (and commit them) as a temporary workaround to get the checks running again until Pylint is fixed:

diff --git a/tox.ini b/tox.ini
index 9f1586c..62995f6 100644
--- a/tox.ini
+++ b/tox.ini
@@ -13,7 +13,7 @@ logstash_async_modules = logstash_async tests
 [testenv]
 deps =
     flake8
-    isort
+    isort<5
     pylint
 commands =
     # linting and code analysis
@@ -29,7 +29,7 @@ commands =
 # for Python 2.7.
 deps =
     flake8
-    isort
+    isort<5
 commands =
     # linting and code analysis
     {envbindir}/flake8 {[tox]logstash_async_modules}

@jloehel
Copy link
Contributor Author

jloehel commented Jul 9, 2020

@eht16 I added the documentation for the new HttpTransport class and I implemented the requested changes. I will add some tests during this week or maybe next week.

Copy link
Owner

@eht16 eht16 left a comment

Choose a reason for hiding this comment

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

Thanks again for your efforts!
Especially for the great docs and consistently using "TLS" 💯

logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
example3.py Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
@jloehel
Copy link
Contributor Author

jloehel commented Jul 9, 2020

Thanks again for your efforts!
Especially for the great docs and consistently using "TLS" 100

You are welcome.

logstash_async/transport.py Outdated Show resolved Hide resolved
logstash_async/transport.py Outdated Show resolved Hide resolved
example3.py Show resolved Hide resolved
@jloehel
Copy link
Contributor Author

jloehel commented Jul 20, 2020

@eht16 Sorry, I was busy last week. I added the the requested changes. I hope I'll find some time to implement the tests this week.

Copy link
Owner

@eht16 eht16 left a comment

Choose a reason for hiding this comment

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

Thanks!
The code looks great now.

I'd be fine with merging this and adding the tests later on.

I also finally managed to release a last Python2 release and a brand-new Python3-only release, so existing Python2 users can stick to the old version and everyone else will get newest and hottest Python3-only code :).

@eht16
Copy link
Owner

eht16 commented Sep 26, 2020

Hi @jloehel,

any news?
Fortunately, the isort vs pylint problem has been resolved in the meantime, so the changes in tox.ini are no longer necessary.

Signed-off-by: Jürgen Löhel <[email protected]>
Signed-off-by: Jürgen Löhel <[email protected]>
Signed-off-by: Jürgen Löhel <[email protected]>
Signed-off-by: Jürgen Löhel <[email protected]>
Signed-off-by: Jürgen Löhel <[email protected]>
Pylint is not yet compatible with isort 5.x
For more information please visit:
  * pylint-dev/pylint#3722
  * pylint-dev/pylint#3725

Signed-off-by: Jürgen Löhel <[email protected]>
Signed-off-by: Jürgen Löhel <[email protected]>
Signed-off-by: Jürgen Löhel <[email protected]>
Signed-off-by: Jürgen Löhel <[email protected]>
This reverts commit f4cd95c.
@jloehel
Copy link
Contributor Author

jloehel commented Sep 27, 2020

Hi @jloehel,

any news?
Fortunately, the isort vs pylint problem has been resolved in the meantime, so the changes in tox.ini are no longer necessary.

Moin moin,

sorry I was some kind of busy the last weeks. I rebased the branch and reverted the isort/py27 commits. May I revert also the type hints and the f-strings?

@eht16
Copy link
Owner

eht16 commented Sep 27, 2020

Hi @jloehel,
any news?
Fortunately, the isort vs pylint problem has been resolved in the meantime, so the changes in tox.ini are no longer necessary.

Moin moin,

sorry I was some kind of busy the last weeks. I rebased the branch and reverted the isort/py27 commits. May I revert also the type hints and the f-strings?

Yes, fine by me, sorry for the trouble. But at least now we are Python3 only.

@jloehel
Copy link
Contributor Author

jloehel commented Sep 28, 2020

Hi @jloehel,
any news?
Fortunately, the isort vs pylint problem has been resolved in the meantime, so the changes in tox.ini are no longer necessary.

Moin moin,
sorry I was some kind of busy the last weeks. I rebased the branch and reverted the isort/py27 commits. May I revert also the type hints and the f-strings?

Yes, fine by me, sorry for the trouble. But at least now we are Python3 only.

Null problemo. ^^ I added the two commits. :-)

@eht16
Copy link
Owner

eht16 commented Oct 3, 2020

Thanks a lot for this feature and your patience with my remarks :).
I squashed most of the commits together to have it a little cleaner in the history and finally merged. Unfortunately, because of the manual squashing, Github doesn't consider this branch as merged but it is.

If you still want to add some tests for the HTTP transport, that'd be fine.

@eht16 eht16 closed this Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants