Skip to content

Conversation

@fundthmcalculus
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2022

Golang Test Report

  2 files  ±0    2 suites  ±0   7s ⏱️ -3s
12 tests ±0    7 ✔️ ±0  0 💤 ±0    5 ±0 
24 runs  ±0  14 ✔️ ±0  0 💤 ±0  10 ±0 

For more details on these failures, see this check.

Results for commit 0eb5e61. ± Comparison against base commit b7cb49e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2022

Python Test Report

  6 files    6 suites   19h 17m 6s ⏱️
10 tests   5 ✔️ 1 💤   4
33 runs  18 ✔️ 3 💤 12

For more details on these failures, see this check.

Results for commit 0eb5e61.

♻️ This comment has been updated with latest results.

}

options.Channel = conn
conn, err := NewServiceConnection(options.ServiceOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

We've made the base class private so that access to the channel property is no longer accessible. Are we not allowing developers to provide a grpc connection for re-use?

Copy link
Contributor

Choose a reason for hiding this comment

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

We removed the channel as direct dependency from service construction to simplify what we pass, but channel is still available for reuse.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good - the way the WithChannel option was handled was to use the channel if provided, and if not we would create a default one.

I understand there could be some confusion if a developer provided both a grpc connection and a "connection config", since we use the provided connection and ignore the config. That being said, I would think a developer who needed to provide a custom grpc connection would not be confused when that was used (the expected behaviour) vs. the config they provided (if they did).

My confusion was just around removing the WithChannel option - since the base implementations are now private channel reuse in golang is no longer possible (you can get the channel, but you cannot provide that to another service when constructing it).

@fundthmcalculus fundthmcalculus enabled auto-merge (squash) March 15, 2022 17:33
@fundthmcalculus fundthmcalculus merged commit c9a5c29 into main Mar 15, 2022
@fundthmcalculus fundthmcalculus deleted the more-go-and-python-fixes branch March 15, 2022 18:04
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.

4 participants