Skip to content

fix(misc): k0sctl migration findings#385

Merged
kke merged 4 commits into
mainfrom
k0sctl-migration-findings
Jun 12, 2026
Merged

fix(misc): k0sctl migration findings#385
kke merged 4 commits into
mainfrom
k0sctl-migration-findings

Conversation

@kke

@kke kke commented Jun 9, 2026

Copy link
Copy Markdown
Contributor
  • The Pipe() helper shell-escapes all arguments via shellescape.Quote, so passing -d"=" produced '-d"="' on the wire, causing cut to receive a three-character delimiter and fail with "the delimiter must be a single character". Use -d= directly, the equals sign needs no shell quoting.
  • DaemonReload no longer exposed via the service wrapper, the caller must know to call the service manager's DaemonReload instead of all callers being forced to call it even-though it's a no-op on all but one of the init systems.
  • UpdateServiceEnvironment returned an error instead of setting service environment variables on windows, which is now done via registry.
  • Remove the http.RoundTripper invention for now - replace with an universal client.Dial(..) later.

Breaking API is not a problem, the v2 API hasn't been released yet.

@kke kke added the bug Something isn't working label Jun 9, 2026
@kke kke force-pushed the k0sctl-migration-findings branch 3 times, most recently from 642356e to 8bbc03d Compare June 11, 2026 09:02
@kke kke marked this pull request as ready for review June 11, 2026 10:42
@kke kke requested a review from Copilot June 11, 2026 10:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses several migration findings by improving cross-platform service management (especially on Windows), fixing a systemd command quoting bug, and simplifying remote HTTP probing by removing the previous http.RoundTripper-style implementation from remotefs.

Changes:

  • Fix systemd service script path resolution by adjusting cut -d argument usage to avoid over-quoting.
  • Make Service.DaemonReload() a no-op when unsupported, and add a Windows SCM path to set service environment variables via registry.
  • Remove the remotefs HTTP transport/RoundTripper approach and replace it with a simpler HTTPStatusInsecure probe API.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
service.go Adds direct env setting via ServiceEnvironmentSetter and makes DaemonReload a no-op when unsupported.
service_test.go Adds tests for the new env-setter path and updates daemon-reload expectations.
remotefs/winfs.go Removes RoundTripper implementation and introduces Windows HTTP status probing via PowerShell.
remotefs/winfs_test.go Removes old HTTP status test tied to the former RoundTripper behavior.
remotefs/upload_test.go Updates test FS stubs to reflect removal of RoundTripper from the FS interface.
remotefs/types.go Replaces HTTPTransport embedding with a narrower Downloader interface.
remotefs/posixfs.go Removes RoundTripper implementation and introduces curl-based HTTP status probing.
remotefs/posixfs_test.go Removes old HTTP status tests tied to the former RoundTripper behavior.
remotefs/patchfile_test.go Updates test FS stubs to reflect removal of RoundTripper from the FS interface.
remotefs/http.go Replaces old HTTP transport helpers with HTTPStatusInsecure + provider interface.
remotefs/http_test.go Replaces extensive RoundTripper tests with targeted HTTPStatusInsecure tests.
initsystem/winscm.go Implements Windows service environment updates via registry (MultiString Environment).
initsystem/winscm_test.go Adds coverage for Windows SCM environment registry updates.
initsystem/systemd.go Fixes cut delimiter arg to avoid Pipe() over-quoting issues.
initsystem/defaultprovider.go Introduces ServiceEnvironmentSetter interface for direct env-setting implementations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread service.go
Comment thread remotefs/http.go
Comment thread remotefs/posixfs.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Comment thread remotefs/http.go
Comment thread remotefs/http.go Outdated
Comment thread remotefs/winfs.go
@kke kke force-pushed the k0sctl-migration-findings branch from 15bde5a to 194f0a1 Compare June 11, 2026 12:58
@kke kke requested a review from Copilot June 11, 2026 13:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comment thread remotefs/posixfs.go Outdated
@kke kke force-pushed the k0sctl-migration-findings branch from 194f0a1 to c9aac6e Compare June 11, 2026 13:49
@kke kke requested a review from Copilot June 11, 2026 13:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread remotefs/http.go Outdated
Comment thread remotefs/http.go
@kke kke force-pushed the k0sctl-migration-findings branch from c9aac6e to 8aa4f13 Compare June 11, 2026 14:25
@kke kke requested a review from Copilot June 11, 2026 14:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Comment thread remotefs/posixfs.go Outdated
Comment thread remotefs/winfs.go Outdated
Comment thread remotefs/winfs.go Outdated
@kke kke force-pushed the k0sctl-migration-findings branch from 8aa4f13 to 8425bbf Compare June 11, 2026 17:57
@kke kke requested a review from Copilot June 11, 2026 17:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread remotefs/http.go Outdated
Comment thread remotefs/http.go Outdated
@kke kke force-pushed the k0sctl-migration-findings branch from 8425bbf to ef53394 Compare June 11, 2026 18:10
@kke kke requested a review from Copilot June 11, 2026 18:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comment thread remotefs/posixfs.go
@twz123

twz123 commented Jun 11, 2026

Copy link
Copy Markdown
Member

DaemonReload should be a no-op on systems that do not have such mechanism (windows)

Wouldn't it be more appropriate to return EWINDOWS or something that is errors.ErrUnsupported, so that callers will know if something actually happened? They can still opt to ignore that error, if that's okay.

@kke

kke commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Wouldn't it be more appropriate to return EWINDOWS or something that is errors.ErrUnsupported, so that callers will know if something actually happened? They can still opt to ignore that error, if that's okay.

It's not windows specific, Systemd is the only one that implements the ServiceManagerReloader interface, OpenRC, runit, upstart, launchd, and whatever there is, don't.

Optimally callers wouldn't need to call it in the first place. The DaemonReload is just mechanics of one init system, sort of an implementation detail when operating with systemd units.

I think the caller doesn't actually care if it worked or "did nothing by design" when the expected happy path result is simply "after I run this, I can start the service, unless it reports an error".

The EWINDOWS / ErrUnsupported would require you to always call it like:

if err := h.DaemonReload(...); err != nil {
  if !errors.Is(err, rig.ErrUnsupported) {
     return err
  }
  // ignore ErrUnsupported
}

I think returning "unsupported" in this case would be more like ErrThanksForCalling.

@kke kke force-pushed the k0sctl-migration-findings branch from 219ba8c to b1b1142 Compare June 12, 2026 09:04
@kke kke requested a review from Copilot June 12, 2026 09:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread service.go
Comment thread remotefs/posixfs.go
@kke

kke commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Maybe the caller shouldn't be calling it.

They can do an interface check on the servicemanager instance and call it. The "wrapper" doesn't need to expose it.

@kke kke force-pushed the k0sctl-migration-findings branch 2 times, most recently from b0c52d6 to 3329912 Compare June 12, 2026 10:46
@kke kke requested a review from Copilot June 12, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread remotefs/http.go Outdated
Comment thread initsystem/winscm.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comment thread service_test.go Outdated
kke added 4 commits June 12, 2026 15:44
The Pipe() helper shell-escapes all arguments via shellescape.Quote, so
passing `-d"="` produced `'-d"="'` on the wire, causing cut to
receive a three-character delimiter and fail with "the delimiter must be
a single character". Use `-d=` directly — the equals sign needs no
shell quoting.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
If the caller wants to explicitly do a DaemonReload, they can
do an interface check against the service manager to see if
ServiceManagerReloader is implemented.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
…egistry

Add ServiceEnvironmentSetter interface for init systems that manage
environment variables directly rather than through a file (e.g. Windows
SCM stores them in HKLM:\SYSTEM\CurrentControlSet\Services\<name>\Environment
as REG_MULTI_SZ). WinSCM implements it via PowerShell New-ItemProperty.

Service.SetEnvironment now prefers ServiceEnvironmentSetter over the
file-based ServiceEnvironmentManager, and returns an error when neither
is supported rather than silently succeeding.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke kke force-pushed the k0sctl-migration-findings branch from 71ad4b0 to b14da94 Compare June 12, 2026 12:45
@kke kke requested a review from Copilot June 12, 2026 12:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

@kke kke merged commit c3b7a51 into main Jun 12, 2026
14 checks passed
@kke kke deleted the k0sctl-migration-findings branch June 12, 2026 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants