Skip to content

Conversation

@bkeryan
Copy link
Collaborator

@bkeryan bkeryan commented Sep 22, 2023

What does this Pull Request accomplish?

service_manager.py:

  • Deprecate public attributes that shouldn't be public
  • Deprecate port attribute and replace it with service_location
  • Remove unnecessary details from docstrings. This class is internal.

service.py:

  • Update MeasurementService to use service_location instead of port
  • Shorten an error message that users are unlikely to encounter
  • Use own DiscoveryClient instead of borrowing GrpcService's DiscoveryClient

Why should this Pull Request be merged?

Hide internal implementation details. The entire GrpcService class is an internal implementation detail, but we didn't make that clear before, so it's better to have a deprecation period before we break compatibility.

Don't assume the server has only one port.

What testing has been done?

Ran poetry run pytest -v

@github-actions
Copy link
Contributor

Test Results

     12 files  ±0       12 suites  ±0   2m 21s ⏱️ +2s
   215 tests ±0     186 ✔️ ±0    29 💤 ±0  0 ±0 
2 568 runs  ±0  2 220 ✔️ ±0  348 💤 ±0  0 ±0 

Results for commit e184df8. ± Comparison against base commit 64310d4.

@bkeryan bkeryan merged commit 93f19ff into main Sep 22, 2023
@bkeryan bkeryan deleted the users/bkeryan/cleanup-service-manager branch September 22, 2023 15:49
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