fix: include log-targets when adding a new layer with harness #1194
Conversation
|
Thanks for the contribution. This is (kind of) mentioned in #1112 (note that "checks" are missing as well as "log-targets"). A couple of things if you'd like to work on this PR further:
Are you willing to work further on this PR to look at it a bit more holistically? |
skatsaounis
force-pushed
the
add-pebble-log-targets-to-layer-harness
branch
2 times, most recently
from
a2ec6bf
to
f3599df
Compare
skatsaounis
force-pushed
the
add-pebble-log-targets-to-layer-harness
branch
from
f3599df
to
96654ff
Compare
|
Hi @benhoyt I think that my two latest commits are taking care of bullets one and three. Thank you for pointing me to the existing issue. I would like to give it a try for the bullet in the middle as well. Should I do it in the same PR or with a follow up? In addition, I think that the static check which is complaining is false positive. However, I am looking forward to a review from a far more experienced pair of eyes to confirm or contradict my suspicion 🙂 |
|
ci errors: /home/runner/work/operator/operator/ops/pebble.py:1088:21 - error: Could not access item in TypedDict
"headers" is not a required key in "HttpDict", so access may result in runtime exception (reportTypedDictNotRequiredAccess)
/home/runner/work/operator/operator/ops/pebble.py:1111:21 - error: Could not access item in TypedDict
"environment" is not a required key in "ExecDict", so access may result in runtime exception (reportTypedDictNotRequiredAccess) |
|
@dimaqq those two failures are exactly what I am thinking that it is a false positive. Being more specific, this is the place where one of them is found: if self.http is None:
self.http = {'headers': {}}
for header, value in other_headers.items():
self.http['headers'][header] = valueUnless I am not mistaken, this |
| if self.http is None: | ||
| self.http = {'headers': {}} | ||
| for header, value in other_headers.items(): | ||
| self.http['headers'][header] = value |
There was a problem hiding this comment.
There's got to be a more elegant way to write this...
| if self.http is None: | |
| self.http = {'headers': {}} | |
| for header, value in other_headers.items(): | |
| self.http['headers'][header] = value | |
| self.http = {**(self.http or {}).get('headers', {}), **other_headers} |
Another option, a little less magical:
| if self.http is None: | |
| self.http = {'headers': {}} | |
| for header, value in other_headers.items(): | |
| self.http['headers'][header] = value | |
| if self.http is None: | |
| self.http = {'headers': {}} | |
| self.http["headers"].update(other_headers) |
As a team, we should actually think about HTTP request header field names, because current API hides the fact that they are case-insensitive by RFC.
Meanwhile a naive merge of {Host:...} + {host:...} will end up with two copies differing by case.
@tonyandrewmeyer wdyt?
There was a problem hiding this comment.
I agree there's got to be a more elegant / lower-code way to write this. For example, if you look at the existing Service._merge, it uses setattr/getattr, but special cases lists and dicts. This makes it significantly less code than the Pebble version.
I think we can take the same approach for LogTarget._merge, and perhaps Check._merge, though the latter has the complication of the nested dicts that we'll need to figure out. Still, it seems to me we can simplify this.
Regarding @dimaqq's point about case sensitivity, I think that's fine as is. Pebble doesn't try to do anything clever here, so we shouldn't either.
There was a problem hiding this comment.
One more concern: is there a way to reset some field?
If a layer specifies http.header.Authorization, can a subsequent layer ever reset auth?
| if other_environment := other.exec.get('environment'): | ||
| if self.exec is None: | ||
| self.exec = {'environment': {}} | ||
| for environment, value in other_environment.items(): | ||
| self.exec['environment'][environment] = value | ||
| if other_user_id := other.exec.get('user-id'): | ||
| if self.exec is None: | ||
| self.exec = {} | ||
| self.exec['user-id'] = other_user_id | ||
| if other_user := other.exec.get('user'): | ||
| if self.exec is None: | ||
| self.exec = {} | ||
| self.exec['user'] = other_user | ||
| if other_group_id := other.exec.get('group-id'): | ||
| if self.exec is None: | ||
| self.exec = {} | ||
| self.exec['group-id'] = other_group_id | ||
| if other_group := other.exec.get('group'): | ||
| if self.exec is None: | ||
| self.exec = {} | ||
| self.exec['group'] = other_group | ||
| if other_working_dir := other.exec.get('working-dir'): | ||
| if self.exec is None: | ||
| self.exec = {} | ||
| self.exec['working-dir'] = other_working_dir |
There was a problem hiding this comment.
There's a bit too much manual code for my liking.
Perhaps we could use getattr/setattr to simplify this over a set of member field names.
When unit testing charms with Harness, if they are adding a new pebble layer with
log-targets, these targets are ignored. This PR is fixing that faulty behavior.The logic is extracted from the pebble code base: