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

Support registration in Consul and created a driver to connect other Services Discovery #221

Merged
merged 28 commits into from
Nov 24, 2020

Conversation

avara1986
Copy link
Member

@avara1986 avara1986 commented Nov 2, 2020

Fixes #190 #178

Changes proposed in this PR:

  • Support registration in Consul
  • Created a driver to connect other services discovery
  • Refactor: added an option to services to initialize actions

@avara1986 avara1986 requested a review from alexppg November 2, 2020 20:08
@avara1986
Copy link
Member Author

@Agalin @alexppg is it ok as a simple service discovery for Consul?

I want to add these days ACL validations and tokens 👍
https://www.consul.io/api-docs/acl/tokens

@coveralls
Copy link

coveralls commented Nov 2, 2020

Pull Request Test Coverage Report for Build 1125

  • 307 of 312 (98.4%) changed or added relevant lines in 35 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 98.998%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyms/cmd/main.py 16 17 94.12%
pyms/flask/app/utils.py 6 7 85.71%
pyms/flask/services/service_discovery.py 39 40 97.5%
pyms/logger/logger.py 8 9 88.89%
tests/test_metrics.py 9 10 90.0%
Files with Coverage Reduction New Missed Lines %
pyms/crypt/driver.py 1 96.97%
Totals Coverage Status
Change from base Build 1088: -0.01%
Covered Lines: 1976
Relevant Lines: 1996

💛 - Coveralls

pyms/flask/services/metrics.py Show resolved Hide resolved
pyms/flask/services/service_discovery.py Show resolved Hide resolved
pyms/flask/services/service_discovery.py Outdated Show resolved Hide resolved
pyms/flask/services/service_discovery.py Outdated Show resolved Hide resolved
pyms/flask/services/service_discovery.py Outdated Show resolved Hide resolved
@Agalin
Copy link
Contributor

Agalin commented Nov 3, 2020

ACL is nice but I wonder how would client-specific configuration options be handled? Is there any plan (maybe in tracer as there are 2 clients already) how should that be done?

@avara1986
Copy link
Member Author

I added all the requests you did @Agalin :)

  • Created a fork of consulate. The Version 0.6.0 isn't work correctly, and not exists new versions since 2015 (0.7 release? gmr/consulate#120)
  • Added a MVP of service discovery with autoregister (optional) but you can access to the client with ms.service_discovery.client.agent.checks() (see examples)
  • Added the option to add your own Service Discovery (see examples)

@avara1986 avara1986 linked an issue Nov 9, 2020 that may be closed by this pull request
@avara1986 avara1986 changed the title WIP: Support registration in Consul and created a driver to conect other Support registration in Consul and created a driver to conect other Nov 9, 2020
@avara1986 avara1986 changed the title Support registration in Consul and created a driver to conect other Support registration in Consul and created a driver to connect other Services Discovery Nov 9, 2020
@Agalin
Copy link
Contributor

Agalin commented Nov 10, 2020

Woah. There is a lot of linter fixes.

Service itself looks fine to me although I've been talking about forking (e.g. into python-microservices org) not vendoring consulate. Vendored is not useful to the community outside of pyms (and for this reason won't get much of external support), harder to provide fixes for (need whole pyms test suite to pass) and increases bundle size for people completely uninterested in service discovery (although this is a minor issue as it doesn't seem to have many dependencies).

@avara1986
Copy link
Member Author

I moved all code into PyMS because I can't upload a new version of consulte to Pip. But... I could create a fork and create a package "pyms-consulate", upload to pip and use it as dependency, in your opinion @Agalin , is it a good solution?

@Agalin
Copy link
Contributor

Agalin commented Nov 10, 2020

That's exactly what I mean by fork in this context. 😄 Just so development on consulate can be continued somewhere.

@alexppg
Copy link
Member

alexppg commented Nov 10, 2020

+1 to forking and have a new package!

@avara1986
Copy link
Member Author

Ready this PR! :)

This is the fork:
https://github.com/python-microservices/consulate
And the package with last changes of consulate:
https://pypi.org/project/py-ms-consulate/

pyms/flask/app/create_app.py Outdated Show resolved Hide resolved
pyms/flask/services/service_discovery.py Outdated Show resolved Hide resolved
tests/test_service_discovery.py Outdated Show resolved Hide resolved
tests/test_service_discovery.py Outdated Show resolved Hide resolved
@avara1986
Copy link
Member Author

do you see anything else @alexppg @Agalin ? is it ready to merge? :)

@Agalin
Copy link
Contributor

Agalin commented Nov 24, 2020

Yeah imo it's fine.

@avara1986 avara1986 merged commit 6cf7e0e into master Nov 24, 2020
@avara1986 avara1986 deleted the feature/issue_190 branch November 24, 2020 19:08
@avara1986 avara1986 linked an issue Nov 28, 2020 that may be closed by this pull request
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.

Support isort Support registration in Consul Support Black
4 participants