Distinguish between LogTargets = nil and LogTargets = []string{}

[barrettj12]

Builds upon #209, addressing Gustavo's review comment here.

Inside the service definition, we need to strike a distinction between

log-targets: ~ # or unspecified
# unmarshals to `LogTargets: nil`

and

log-targets: []
# unmarshals to `LogTargets: []string{}`

Nil LogTargets means the service has not specified log targets, so its logs will be forwarded to all the default/opt-out log targets. In contrast, log-targets: [] means the service has explicitly specified no log targets, so its logs should not be forwarded anywhere.

This PR amends the service.LogsTo function to reflect this change. We just need to check if s.LogTargets == nil instead of checking len(s.LogTargets) == 0 (and update the test to match).

We also need to ensure that we override nil/empty log targets correctly. We use the following rules:

1. merging nil <- [] should result in []
2. replacing nil <- [] should result in []
3. merging [] <- nil should result in []
4. replacing [] <- nil should result in nil

The Service.Copy and Service.Merge methods needed to be updated to obey these rules. To this end, I've created a copyStrings utility method to copy a string slice in such a way that preserves the difference between []string(nil) and []string{}.

Finally, we add two extra plan tests to:

• check various ways of specifying nil/empty log-targets in the plan
• check merging of nil/empty log targets.

[niemeyer]

The logic seems mathematically sound, but unfortunately we still have an important usability issue, which is rooted on the fact that nil is a default for any unset field. So let's say someone sets the field to [] in a given layer. How would we reset that field back to an unset value so that we might once again benefit from using the general logging options? It's impossible without a replacement, except a replacement will reset the entire service and not just the logging. So in practice, it's impossible to achieve that important value without also resetting the entire service, which kills the rationale why we have that mechanism.

This PR is still great, but let's try to find a way to have it fixing this issue too.

[barrettj12]

@niemeyer I understand the rationale for the log-targets: ~ behaviour as follows. You can take your existing plan and add a new layer on top:

log-targets:
  loki:
    ...

and have all your services' logs sent to Loki without having to modify the existing services. For anything more complex than this, IMO you should be specifying log targets explicitly in each service.

How would we reset that field back to an unset value so that we might once again benefit from using the general logging options?

The workaround here is to explicitly specify the log targets you want to forward to. I see the log-targets: ~ behaviour as a "fallback" that makes it easy to start log forwarding (just define a target - voila). It's not intended for complex use cases. IMO once a user starts explicitly specifying log targets, they are agreeing to explicitly specify log targets in all future layers.

Basically any forwarding configuration is possible, by explicitly specifying the right log-targets for every service.

It's impossible without a replacement, except a replacement will reset the entire service and not just the logging.

True, but this is a more general problem. If my service specifies

log-targets: [tgt1, tgt3]

but I want to instead forward to

log-targets: [tgt1, tgt2]

then I am forced to use override: replace and write out the full service definition.

This problem is not isolated to log targets either. If I want to remove a service dependency (after, before, requires) or an environment variable, then I am forced to replace the entire service.

The best solution here is to specify override on the level of individual fields in the service spec, not for the service overall. But this is a pretty fundamental change to Pebble configuration.

We can discuss this further at the spec review meeting next week.

[niemeyer]

This seems out-of-date considering recent conversations. I'm closing it for the time being.