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.

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

Use the correct template when installing as a SystemV service #5044

Merged
merged 7 commits into from
Sep 30, 2024

Conversation

kke
Copy link
Contributor

@kke kke commented Sep 26, 2024

Description

Several fixes in separate commits as preparation for #4962

The first one fixes a bug and the PR title has been derived from that. The rest are just housekeeping.

Use SysVScript for system v, not SystemdScript

The customized system V service template from pkg/install/linux_sysv.go was not being used, as the
template was assigned to SystemdScript option. The correct option name seems to be SysVScript.

Remove double "name" field from openrc script

The openrc unit file template had the "name=" definition twice. I assume
it is the last one that was effective.

Extract systemd template to pkg/install/linux_systemd.go

Unlike OpenRC, Upstart and SystemV unit file templates, the systemd
template was defined in pkg/install/service.go. Here it has been extracted
into a dedicated file like the other templates.

Remove empty pkg/install/process.go

The file was empty except for the copyright header and package name.

Remove unused darwin launchd integration

Since k0s does not run on mac, there's no need for launchd service
installer.

Remove unused install.GetSysInit() function

There are no references to this function.

Remove unused prepareEnvVars function

There are no references to this function.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@kke kke added bug Something isn't working chore area/cli area/install labels Sep 26, 2024
There are no references to this function.

Signed-off-by: Kimmo Lehto <[email protected]>
Since k0s does not run on mac, there's no need for launchd service
installer.

Signed-off-by: Kimmo Lehto <[email protected]>
The file was empty except for the copyright header and package name.

Signed-off-by: Kimmo Lehto <[email protected]>
Unlike openrc, upstart and system v unit file templates, the systemd
template was defined in pkg/install/service.go. Here it is extracted
into a dedicated file like the other templates.

Signed-off-by: Kimmo Lehto <[email protected]>
The customized system V service template was not being used, as the
template was put into "SystemdScript" option. The correct option name
seems to be "SysVScript".

Signed-off-by: Kimmo Lehto <[email protected]>
The openrc unit file template had the "name=" definition twice. I assume
it is the last one that was effective.

Signed-off-by: Kimmo Lehto <[email protected]>
There are no references to this function. The linter catches is as
unused.

Signed-off-by: Kimmo Lehto <[email protected]>
@kke kke changed the title Service installation clean up and fixes Use the correct template when installing as a SystemV service Sep 26, 2024
@kke kke marked this pull request as ready for review September 26, 2024 13:20
@kke kke requested a review from a team as a code owner September 26, 2024 13:20
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would make more sense to put the template into its dedicated file and use go:embed to put it into a string variable somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem to be an established filename extension for go's text/template format files, if there was, it would be nice for editor benefits.

Copy link
Member

Choose a reason for hiding this comment

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

https://marketplace.visualstudio.com/items?itemName=jinliming2.vscode-go-template

Seems to hint towards using *.go.txt, *.go.tpl, *.go.tmpl, *.gtpl.

@jnummelin jnummelin added backport/release-1.28 PR that needs to be backported/cherrypicked to release-1.28 branch backport/release-1.29 PR that needs to be backported/cherrypicked to the release-1.29 branch backport/release-1.30 PR that needs to be backported/cherrypicked to the release-1.30 branch labels Sep 30, 2024
@jnummelin jnummelin merged commit 9ea9b88 into main Sep 30, 2024
90 checks passed
@jnummelin jnummelin deleted the k0s-install-cleanup-fixes branch September 30, 2024 08:37
@k0s-bot
Copy link

k0s-bot commented Sep 30, 2024

Successfully created backport PR for release-1.28:

@k0s-bot
Copy link

k0s-bot commented Sep 30, 2024

Successfully created backport PR for release-1.29:

@k0s-bot
Copy link

k0s-bot commented Sep 30, 2024

Successfully created backport PR for release-1.30:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli area/install backport/release-1.28 PR that needs to be backported/cherrypicked to release-1.28 branch backport/release-1.29 PR that needs to be backported/cherrypicked to the release-1.29 branch backport/release-1.30 PR that needs to be backported/cherrypicked to the release-1.30 branch bug Something isn't working chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants