Implement Service Checks For Windows#1017
Conversation
…art implementing service logic now
petemounce
left a comment
petemounce
left a comment
There was a problem hiding this comment.
@cthiel42 thanks very much for your contribution, and apologies for latency on review. This looks broadly good to me. I think ideally it'd be conditionally compiled so it's only present within the Windows binary to follow the pattern the other OS-specific support established.
re: my suggestions - please consider them my own in-memory suggestions for changes as opposed to relying on them to be syntactically / semantically correct. I made them inline during the review, not via an editor.
@aelsabbahy any comments?
|
@petemounce Thanks for the review. I'll take a look over the next few days and push some changes for your comments. |
|
@petemounce I pushed up some changes. I switched the powershell calls to be through a helper function. I removed the comparison operator from the powershell commands and put them in the code (thanks for that suggestion, it looks a lot cleaner to me). Lastly, the windows service file is now not included in linux builds. On that last item, I accomplished that by stubbing out the call to NewServiceWindows. That's a functioning way for the linux builds to run with that call in there. Another way would be to take |
petemounce
left a comment
petemounce
left a comment
There was a problem hiding this comment.
@cthiel42 thanks very much for your increment, and apologies again for latency. Couple more suggestions.
I see I now have an Approve workflows to run button, so I've blithely pressed that and can see GHA has done its thing. Not sure if that button is needed each time; will see. Still no Travis, though; I lack access to address that.
I'd be cool with this merging once CI is run and green. @ripienaar / @aelsabbahy, any thoughts?
| func NewCommandForWindowsPowershell(name string, arg ...string) *Command { | ||
| //fmt.Println(arg) | ||
| command := new(Command) | ||
| command.name = "powershell" |
There was a problem hiding this comment.
Maybe not this PR, but I gather that powershell-core's binary is pwsh.
Is there a way this choice might be later threaded through without a breaking change? I guess it would be a property on the service like powershell-binary, defaulting to powershell...?
There was a problem hiding this comment.
Are you thinking of using pwsh if its available, and falling back to powershell if its not available?
There was a problem hiding this comment.
No, I was thinking of this being left hard-coded to powershell and then (later in a follow-up) providing the ability to a test-author to override that with (something like) powershell_binary: pwsh in the test YAML definition. If that test-attribute is omitted, the default no-value choice would be powershell as in this PR.
There was a problem hiding this comment.
I haven't looked too much into this, but I think as long as we can get the config to be accessible within this method, we could do that.
|
@cthiel42 would you mind addressing https://github.com/goss-org/goss/actions/runs/21811022610/job/63155554726?pr=1017#step:4:34, which ought to just be a |
@petemounce This should hopefully be resolved now |
2 participants
Checklist
make test-all(UNIX) passes. CI will also test thisDescription of change
I noticed some of the functionality for Windows wasn't implemented when I was using Goss with Windows a few months ago and I'd like to implement some of it, starting with services.
I tried to keep the initial changes here simple to see what you were looking for. For instance, services and processes are set up a little different than the others, so there would need to be some small refactoring if you wanted the windows service file to only be compiled into the windows build. I have an idea about how it could be accomplished somewhat cleanly, but wanted to get your take on whether it was worth it or not first. Same goes for integration tests. There was one test already in there, let me know if you wanted more and what you want them to cover.