cmanaha / python-elasticsearch-logger

Python Elasticsearch handler for the standard python logging framework

Other

232 stars 116 forks source link

Adding aws eleasticsearch authentication #15

Closed KonstantinSchubert closed 7 years ago

KonstantinSchubert commented 7 years ago

Feel free to request changes or edit my pull request before merging.

cmanaha commented 7 years ago

Looks pretty straightforward to me; Thanks a lot for the pull-request and contribution Some tests are not passing (Definition list ends without a blank line; unexpected unindent upon tox execution) I'll take a look, test it and merge it. If everything goes well I'll also force a package release as there might be others interested on this!

KonstantinSchubert commented 7 years ago

Hi, thanks for your kind response :) Should I fix the formatting problems or are you taking care of this?

cmanaha commented 7 years ago

Go for it

Otherwise I'll pick it up as soon I have some spare time

Carlos

On 31 Mar 2017, at 15:29, Konstantin Schubert notifications@github.com wrote:

Should I fix the formatting problems or are you taking care of this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

KonstantinSchubert commented 7 years ago

Frankly I am not sure what the right format is for these comments and how I can lint them. It would take me a while to figure this out.

cmanaha commented 7 years ago

That's fine. I'll take care of it as soon as I have some spare time.

On 3 Apr 2017, at 12:43, Konstantin Schubert notifications@github.com wrote:

Frankly I am not sure what the right format is for these comments, so it would take me a long time to figure this out. I think for you it is just a minute or two.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

codecov-io commented 7 years ago

Codecov Report

Merging #15 into master will decrease coverage by 4.06%. The diff coverage is 52.38%.

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   87.39%   83.33%   -4.07%     
==========================================
  Files           1        1              
  Lines         119      138      +19     
  Branches       14       17       +3     
==========================================
+ Hits          104      115      +11     
- Misses         13       21       +8     
  Partials        2        2

Impacted Files	Coverage Δ
cmreslogging/handlers.py	`83.33% <52.38%> (-4.07%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f047106...23fbc6a. Read the comment docs.

KonstantinSchubert commented 7 years ago

@cmanaha It looks like I have to add some tests? If you suggest a good test I can make, I will do it.

cmanaha commented 7 years ago

@KonstantinSchubert on the testing issues; Pretty much passes coverage and what is missing is integration tests for authentication of all kinds; This are difficult to test on free continuous integration systems (except may be the basic auth on top of ES) specially as they require a Kerberos setup and IAM details to consider. If you have any thoughts on how they will be welcome. Otherwise only option is to test the integration manually; By the way, I released your changes on to pypi https://pypi.python.org/pypi/CMRESHandler/1.0.0b3

KonstantinSchubert commented 7 years ago

:+1: