Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snappy: refactor ServiceDescription away #1114
Conversation
zyga
added some commits
May 2, 2016
pedronis
reviewed
May 2, 2016
| -Description={{.Description}} | ||
| -After=snapd.frameworks.target{{ if .Socket }} {{.SocketFileName}}{{end}} | ||
| -Requires=snapd.frameworks.target{{ if .Socket }} {{.SocketFileName}}{{end}} | ||
| +Description=service {{.App.Name}} for snap {{.App.Snap.Name}} - autogenerated DO NO EDIT |
pedronis
reviewed
May 2, 2016
| X-Snappy=yes | ||
| [Service] | ||
| -ExecStart=/usr/bin/ubuntu-core-launcher {{.UdevAppName}} {{.AaProfile}} {{.FullPathStart}} | ||
| +ExecStart=/usr/bin/ubuntu-core-launcher {{.App.SecurityTag}} {{.App.SecurityTag}} {{.FullPathStart}} | ||
| Restart={{.Restart}} | ||
| WorkingDirectory=/var{{.SnapPath}} |
pedronis
May 2, 2016
Contributor
well, then do like for SnapPath for now, but the /var is a hack making assumptions
niemeyer
reviewed
May 2, 2016
| serviceTemplate := `[Unit] | ||
| -Description={{.Description}} Socket Unit File | ||
| +Description=socket {{.App.Name}} for snap {{.App.Snap.Name}} - autogenerated DO NO EDIT |
niemeyer
May 2, 2016
Contributor
Socket {{.App.Name}} for snap {{.App.Snap.Name}}
Capital-cased like other service/socket file, and no need to say auto-generated. These files usually come inside the package, and people are used to not fiddling with them.
niemeyer
May 2, 2016
Contributor
Hmm.. that will be a weird sentence depending on the name of the snap and app.
How about:
Socket for snap application {{.App.Snap.Name}}.{{.App.Name}}
niemeyer
reviewed
May 2, 2016
| restartCond, | ||
| - desc.Type, | ||
| + serviceStopTimeout(appInfo), | ||
| + appInfo.Snap.Version, |
zyga
May 2, 2016
Contributor
This is a separate refactor I'm already looking at. This is because dedicated fields are required by snapenv API today.
pedronis
May 2, 2016
Contributor
the first falls back to a default timeout which is a piece of policy that is unclear if it belongs to snap/ but it could be done with other tools offered by templating starting from the value in App
zyga
May 2, 2016
•
Edited 1 time
-
zyga
May 2, 2016
Contributor
-
May 2, 2016
I plan to make a small refactoring branch that will move such assumptions to snap.yaml parsing. It is less harmful if defaults are applied in one spot only.
niemeyer
May 2, 2016
Contributor
Isn't this actually the same refactoring being done in this branch? I mean, this is just about using {{.App.Snap.Version}} in that template instead of {{Version}}, isn't it?
pedronis
May 2, 2016
Contributor
I think he is talking about serviceStopTimeout, I have mixed feelings about making that a snap/ concern
pedronis
reviewed
May 2, 2016
| + // service file, this ensures that /snap/foo/1.0/bin/start | ||
| + // is in the service file when the SetRoot() option | ||
| + // is used | ||
| + realBaseDir := stripGlobalRootDir(baseDir) |
pedronis
May 2, 2016
Contributor
given that u-d-f doesn't do this, this is mostly for the benefit of the tests wondering if there is some other way around that, anyway, may be for a later branch
zyga
May 2, 2016
•
Edited 1 time
-
zyga
May 2, 2016
Contributor
-
May 2, 2016
I kept this code around as-is but personally I think we should not do this. If you agree I can remove it and adjust tests (though tests will look more verbose)
zyga
added some commits
May 2, 2016
|
LGTM |
zyga commentedMay 2, 2016
This branch, through successive small changes, entirely removes
snappy.ServiceDescriptionsince it can be completely derived fromsnap.AppInfo