rest-proxy: split out into standalone module #993
Conversation
nosahama
force-pushed
the
nosahama/split-out-rest-proxy
branch
9 times, most recently
from
daccba7 to
7b9bb5c
Compare
nosahama
force-pushed
the
nosahama/split-out-rest-proxy
branch
from
7b9bb5c to
3cf0f16
Compare
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with closing(arg.config_file): | ||
| config = read_config(arg.config_file) |
There was a problem hiding this comment.
if you aren't using the closing arg I don't think you are closing it correctly.
Shouldn't be:
with closing(foo) as bar:
config = read_config(bar)?
There was a problem hiding this comment.
I can take a look, once again, this is existing code 😉
| if "password" in key: | ||
| value = "****" |
There was a problem hiding this comment.
this is dangerous, you should keep the info about the secrecy of the field in the context of where its created. Otherwise if tomorrow we add another field like e.g. sentry_api_token this isn't filtered and there isn't any hint about this indirect dependency.
I suggest to add the filtering in the object itself, you can add to the Config object a method like:
SENSITIVE_FIELDS: Final = ("registry_password", "ssl_password", "sasl_plain_password", "sasl_oauth_token")
@staticmethod
def is_field_a_secret(self, field_name: str) -> bool:
if field_name not in Config:
raise ValueError(f"{field_name} is not known")
return field_name not in SENSITIVE_FIELDS
In that way the filtering logic its "near" to where the addition/removal/renaming of the property is, its a property of the data structure rather than a piece of coupled code put randomly in another file and that the developer should know to look before adding/removing something far away.
PS I think the logic wasn't covering the sasl_oauth_token
There was a problem hiding this comment.
Once again, this is existing code 😉
I'd reason around your suggestions and see to add/fix or create follow up items 👍
| for ((i = 0; i <= retries; i++)); do | ||
| response=$(curl --silent --verbose --fail http://localhost:8083/topics) | ||
|
|
||
| if [[ $response == '["_schemas","__consumer_offsets"]' ]]; then |
There was a problem hiding this comment.
Would __consumer_offsets be enough here, the _schemas is schema registry special.
There was a problem hiding this comment.
I think testing against _schemas adds a little bit more coverage as that topic is created from application code and truly verifies the flow.
There was a problem hiding this comment.
Also this is existing code/logic.
| @@ -27,6 +27,22 @@ rest) | |||
| echo "Starting Karapace REST API" | |||
| exec python3 -m karapace.karapace_all /opt/karapace/rest.config.json | |||
| ;; | |||
| rest_proxy) | |||
There was a problem hiding this comment.
The case rest) is not required anymore.
|
Setting as draft PR, so I can run some system test against this hash. |
About this change - What it does
Make
rest-proxycomponent standalone module still running therapubased webserver, leaving onlyschema_registryand shared components insrc/karapace.Why this way
We can make future changes that aid in providing the karapace components as separate services.