Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: include log-targets when adding a new layer with harness #1194

Closed
wants to merge 2 commits into from
Closed

fix: include log-targets when adding a new layer with harness #1194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f68d41
fix: include log-targets when adding a new layer with harness
skatsaounis Apr 19, 2024
96654ff
fix: add missing checks from add_layer function
skatsaounis Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions ops/pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,74 @@ def to_dict(self) -> 'CheckDict':
dct = {name: value for name, value in fields if value}
return typing.cast('CheckDict', dct)

def _merge(self, other: 'Check'):
"""Merges this check object with another check definition.

For attributes present in both objects, the passed in check
attributes take precedence.
"""
if other.level != '':
self.level = other.level
if other.period != '':
self.period = other.period
if other.timeout != '':
self.timeout = other.timeout
if other.threshold is not None:
self.threshold = other.threshold
if other.http is not None:
if other_url := other.http.get('url'):
if self.http is None:
self.http = {}
self.http['url'] = other_url
if other_headers := other.http.get('headers'):
if self.http is None:
self.http = {'headers': {}}
for header, value in other_headers.items():
self.http['headers'][header] = value
Comment on lines +1085 to +1088
Copy link
Contributor

Choose a reason for hiding this comment

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

There's got to be a more elegant way to write this...

Suggested change
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:

Suggested change
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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.tcp is not None:
if other_port := other.tcp.get('port'):
if self.tcp is None:
self.tcp = {}
self.tcp['port'] = other_port
if other_host := other.tcp.get('host'):
if self.tcp is None:
self.tcp = {}
self.tcp['host'] = other_host
if other.exec is not None:
if other_command := other.exec.get('command'):
if self.exec is None:
self.exec = {}
self.exec['command'] = other_command
if other_service_context := other.exec.get('service-context'):
if self.exec is None:
self.exec = {}
self.exec['service-context'] = other_service_context
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
Comment on lines +1107 to +1131
Copy link
Contributor

@dimaqq dimaqq May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


def __repr__(self) -> str:
return f'Check({self.to_dict()!r})'

Expand Down Expand Up @@ -1117,6 +1185,23 @@ def to_dict(self) -> 'LogTargetDict':
dct = {name: value for name, value in fields if value}
return typing.cast('LogTargetDict', dct)

def _merge(self, other: 'LogTarget'):
"""Merges this log target object with another log target definition.

For attributes present in both objects, the passed in log target
attributes take precedence.
"""
if other.type != '':
self.type = other.type
if other.location != '':
self.location = other.location
self.services.extend(other.services)
if other.labels is not None:
if self.labels is None:
self.labels = {}
for name, value in other.labels.items():
self.labels[name] = value

def __repr__(self):
return f'LogTarget({self.to_dict()!r})'

Expand Down
30 changes: 30 additions & 0 deletions ops/testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3067,6 +3067,36 @@ def add_layer(
s._merge(service)
else:
layer.services[name] = service
for name, log_target in layer_obj.log_targets.items():
if not log_target.override:
raise RuntimeError(f'400 Bad Request: layer "{label}" must define'
f'"override" for log target "{name}"')
if log_target.override not in ('merge', 'replace'):
raise RuntimeError(f'400 Bad Request: layer "{label}" has invalid '
f'"override" value for log target "{name}"')
elif log_target.override == 'replace':
layer.log_targets[name] = log_target
elif log_target.override == 'merge':
if combine and name in layer.log_targets:
l_t = layer.log_targets[name]
l_t._merge(log_target)
else:
layer.log_targets[name] = log_target
for name, check in layer_obj.checks.items():
if not check.override:
raise RuntimeError(f'400 Bad Request: layer "{label}" must define'
f'"override" for check "{name}"')
if check.override not in ('merge', 'replace'):
raise RuntimeError(f'400 Bad Request: layer "{label}" has invalid '
f'"override" value for check "{name}"')
elif check.override == 'replace':
layer.checks[name] = check
elif check.override == 'merge':
if combine and name in layer.checks:
c = layer.checks[name]
c._merge(check)
else:
layer.checks[name] = check

else:
self._layers[label] = layer_obj
Expand Down
Loading
Loading