Add options to configure applications socket activation

[albertodonato]

This adds schema and rendering for the sockets entries in apps, to configure socket activations for deamons. See snapcore/snapd#3916 for the snapd implementation

[kyrofa]

This is looking pretty good, thanks @albertodonato! What happens if a snap built with this is run against a released snapd (that doesn't support it)? I have a few more comments inline.

[kyrofa]

This isn't quite the same as that used in the snapd PR to validate socket names: ^(?:[a-z0-9]+-?)*[a-z](?:-?[a-z0-9])*$. Should the other PR be using the same regex as app names, perhaps?

[albertodonato]

Fixed to use the same regexp as the one for the snap name (as the snapd PR does)

[kyrofa]

As it is, let's say I use a value that doesn't match this schema, say, listen-stream: true. That will result in the following dreadfully unhelpful error:

Issues while validating snapcraft.yaml: The 'apps/foo/sockets/bar/listen-stream' property does not match the required schema: True is not valid under any of the given schemas

This is of course not your fault, but it's something we're trying to improve. For example, if you change it to e.g.

properties:
  listen-stream:
    anyOf:
      - type: integer
        usage: "port number, an integer between 1 and 65535"
        minimum: 1
        maximum: 65535
      - type: string
        usage: "socket path, a string"

The error message is immediately more helpful:

Issues while validating snapcraft.yaml: The 'apps/foo/sockets/bar/listen-stream' property does not match the required schema: True is not valid under any of the given schemas (must be one of 'port number, an integer between 1 and 65535' or 'socket path, a string')

[albertodonato]

Nice, I didn't know about this. changed as suggested

[kyrofa]

Nice job keeping the octal representation, here.

[kyrofa]

There are some socket options documented here: https://github.com/canonical-docs/snappy-docs/blob/master/build-snaps/syntax.md . It looks like that stuff needs to be updated with this, if you're up for it.

[albertodonato]

@kyrofa thanks for the review. All comments should be addressed, I will make a PR for the documentation update once it (and the snapd counterpart) lands

[albertodonato]

@kyrofa WRT a snap using the sockets options with a snapd that doesn't support it, I think the option will be silently ignored

[kyrofa]

This pattern is supposed to correspond to this, no? The pattern I see here would allow just numbers, for example.

[albertodonato]

Yes, it should correspond to that. I copied this from the snap name, attribute; doesn't that mean that a snap name with just numbers would also be allowed?

[kyrofa]

Yes indeed, it appears snapcraft and snapd have diverged on this point. I suggest using the correct pattern here, I'll fix the snap name in another PR.

[kyrofa]

Now that #1615 has landed, I have another suggestion. When the schema is:

1. An object
2. With no additional properties allowed
3. That uses patternProperties rather than hard-coded properties

... and is handed an object with properties that do not match the pattern, rather than barfing on the pattern, jsonschema unhelpfully barfs on the additionalProperties: false instead. So a YAML like this:

apps:
  foo:
    command: echo "hello"
    sockets:
      BAD_SOCKET:
        listen-stream: bar

Results in this:

Issues while validating snapcraft.yaml: The 'apps/foo/sockets' property does not match the required schema: Additional properties are not allowed ('BAD_SOCKET' was unexpected)

That can be improved if you add a validation-failure message like this:

sockets:
  type: object
  additionalProperties: false
  validation-failure:
    "{!r} is not a valid socket name. Socket names consist of
    lower-case alphanumeric characters and hyphens."
  patternProperties:
    "^[a-z0-9][a-z0-9+-]*$":
      <snip>

Then the error message becomes:

Issues while validating snapcraft.yaml: The 'apps/foo/sockets' property does not match the required schema: 'BAD_SOCKET' is not a valid socket name. Socket names consist of lower-case alphanumeric characters and hyphens.

[albertodonato]

updated schema as suggested

[kyrofa]

GitHub is suffering from a queue backup this morning, but once tests pass here, +1 from me. Thanks @albertodonato 😃 .

[sergiusens]

On mar, oct 17, 2017 at 11:45 AM, Kyle Fazzari ***@***.***> wrote: @kyrofa approved this pull request. GitHub is suffering from a queue backup this morning, but once tests pass here, +1 from me. Thanks @albertodonato 😃 .

Thanks, while the fix is correct, given that we are not adding an `assumes` entry when using this keyword, I am not confident of landing this before it is generally available in all distros.

…

[kyrofa]

Thanks, while the fix is correct, given that we are not adding an assumes entry when using this keyword, I am not confident of landing this before it is generally available in all distros.

Fine by me, although I feel like that's the developer's job, not ours. We don't add assumes for hooks, either, but we support creating snaps that use them. I think it makes sense to wait for the snapd PR to land in case it changes, but actually waiting for it to be released means no one can test that feature before it is unless they want to make snaps by hand.

[sergiusens]

That is a maintenance burden and we can and should do better. We have not so far done this kind of thing where we released a version of snapcraft before the store or snapd side of the feature was available.

[albertodonato]

@sergiusens @kyrofa can this be merged now that the snapd change has landed?

[sergiusens]

On mar, nov 14, 2017 at 4:36 PM, Alberto Donato ***@***.***> wrote: @sergiusens @kyrofa can this be merged now that the snapd change has landed?\

in stable?

…

[albertodonato]

no, it was merged to master

[kyrofa]

@albertodonato as @sergiusens mentioned above, we don't want to support this until the feature is available in a stable snapd release.