-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
chore(windows): Rework Windows service handling #15372
base: master
Are you sure you want to change the base?
Conversation
Will this be an important/breaking changelog change? As the --service to service will likely break deploy scripts/pipelines |
@Hr0bar of course |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the (what looks like) backwards compatible commands and the cleanup of reading the config.
Played with this locally a bit and looked good. I think we should clean up the service
subcommand output a bit, no need to echo what we are going to do back to the user, and there always appears to be an extra newline?
cmd/telegraf/cmd_win_service.go
Outdated
`, | ||
Action: func(cCtx *cli.Context) error { | ||
name := cCtx.String("service-name") | ||
fmt.Fprintf(outputBuffer, "Querying status of service %s...\n", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove these as the user should know what they are doing if they run the subcommand status
.
cmd/telegraf/cmd_win_service.go
Outdated
`, | ||
Action: func(cCtx *cli.Context) error { | ||
name := cCtx.String("service-name") | ||
fmt.Fprintf(outputBuffer, "Stopping service %s...\n", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove, don't need to repeat what the user told us to do.
cmd/telegraf/cmd_win_service.go
Outdated
`, | ||
Action: func(cCtx *cli.Context) error { | ||
name := cCtx.String("service-name") | ||
fmt.Fprintf(outputBuffer, "Uninstalling service %s...\n", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
cmd/telegraf/cmd_win_service.go
Outdated
configDirs: cCtx.StringSlice("config-directory"), | ||
} | ||
name := cCtx.String("service-name") | ||
fmt.Fprintf(outputBuffer, "Installing service %s...\n", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
`, | ||
Action: func(cCtx *cli.Context) error { | ||
name := cCtx.String("service-name") | ||
fmt.Fprintf(outputBuffer, "Starting service %s...\n", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Fprintf(outputBuffer, "Starting service %s...\n", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srebhan can we get rid of this one and call it good?
Summary
This PR reworked the Windows service handling of Telegraf, including the replacing a 3rd-party library with standard calls as well as introducing the
service
command instead of the--service flag
.Checklist
Related issues
resolves #14834