-
Notifications
You must be signed in to change notification settings - Fork 5
[DPE-3689, DPE-4179] Expose read-write and read-only endpoints when related to data-integrator + TLS support #119
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
Changes from 3 commits
0e93ef5
b4f85d0
ad8b129
6dae91c
716da08
8293c94
5b3627b
56c0c94
61e2c8c
6b47d09
eddf296
5fa3bc7
26235fd
3657e83
b7afd29
d9c072c
79fd8ef
ee4b098
6f4b271
eba3c8f
2e276ba
0cf8b71
0a36768
1134e43
41d7616
6f60488
40beb97
1b0bb19
996ba1e
b99ecce
10748cf
03d4ed4
0978187
f8828dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,10 +109,9 @@ def _exposed_read_write_endpoint(self) -> str: | |
def _exposed_read_only_endpoint(self) -> str: | ||
"""The exposed read-only endpoint""" | ||
|
||
@property | ||
@abc.abstractmethod | ||
def is_exposed(self) -> typing.Optional[bool]: | ||
"""Whether router is exposed externally""" | ||
def is_externally_accessible(self, event=None) -> typing.Optional[bool]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: please remove default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in 0978187 |
||
"""Whether router is externally accessible""" | ||
shayancanonical marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is this only used by vm charm? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it is only used by the vm charm. but needs to be defined in abstract_charm because reconcile in workload calls to it. in vm, it will return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you mention that it's only used by machine charm in docstring? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in 0978187 |
||
|
||
@property | ||
def _tls_certificate_saved(self) -> bool: | ||
|
@@ -269,7 +268,9 @@ def reconcile(self, event=None) -> None: # noqa: C901 | |
if self._upgrade.unit_state == "outdated": | ||
if self._upgrade.authorized: | ||
self._upgrade.upgrade_unit( | ||
workload_=workload_, tls=self._tls_certificate_saved | ||
workload_=workload_, | ||
tls=self._tls_certificate_saved, | ||
exporter_config=self._cos_exporter_config(event), | ||
) | ||
else: | ||
self.set_status(event=event) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,16 +83,15 @@ def _exposed_read_write_endpoint(self) -> str: | |
def _exposed_read_only_endpoint(self) -> str: | ||
return f"{self.host_address}:{self._READ_ONLY_PORT}" | ||
|
||
@property | ||
def is_exposed(self) -> typing.Optional[bool]: | ||
return self._database_provides.external_connectivity | ||
def is_externally_accessible(self, event=None) -> typing.Optional[bool]: | ||
return self._database_provides.external_connectivity(event) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in 0978187 |
||
|
||
def _reconcile_node_port(self, event) -> None: | ||
"""Only applies to Kubernetes charm, so no-op.""" | ||
pass | ||
|
||
def _reconcile_ports(self) -> None: | ||
if self.is_exposed: | ||
if self.is_externally_accessible(): | ||
shayancanonical marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ports = [self._READ_WRITE_PORT, self._READ_ONLY_PORT] | ||
else: | ||
ports = [] | ||
|
@@ -108,7 +107,7 @@ def wait_until_mysql_router_ready(self) -> None: | |
wait=tenacity.wait_fixed(5), | ||
): | ||
with attempt: | ||
if self.is_exposed: | ||
if self.is_externally_accessible(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does event need to be passed here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in 0978187 |
||
for port in ( | ||
self._READ_WRITE_PORT, | ||
self._READ_ONLY_PORT, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this file shared across vm & k8s? if not, can it be? it appears to be nearly identical IMO, we should get back to the pattern that if the file name is the same on the vm and k8s charm, the file is identical on both charms. Otherwise, behavior across vm & k8s router will diverge & the maintenance cost will increase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i agree with you, but imo, there's some divergence between the vm and k8s router charms. i would be an advocate of separating shared files and converging the two charms as a separate effort that fast follows up this PR |
Uh oh!
There was an error while loading. Please reload this page.