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.

Sign up for GitHub

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

Jump to bottom

Allow running as console application on Windows #2754

Merged
merged 4 commits into from
Jan 25, 2018
Merged

Allow running as console application on Windows #2754

merged 4 commits into from
Jan 25, 2018

Conversation

MondayHopscotch
Copy link
Contributor

@MondayHopscotch MondayHopscotch commented May 2, 2017 •
edited
Loading

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • [N/A] README.md updated (if adding a new plugin)

Allow windows service to run as console application

@aMaZing0x41
Copy link

👍 This is useful when one does not necessarily want to run telegraf as a stand-alone native service. It will essentially allow one to bypass telegraf running as a service so that it can be invoked by external applications. This change was primarily introduced in PR #1543.

@danielnelson
Copy link
Contributor

It seems to me that the absence of the -service flag should cause Telegraf to run in the foreground, and if Telegraf is in the foreground there should be a console window.

@danielnelson danielnelson added this to the 1.4.0 milestone May 2, 2017
@aMaZing0x41
Copy link

@danielnelson I don't believe that the -service flag is in play here. Currently telegraf will always attempt to run as a service when using a non-interactive login. There is no way to override this behavior.
If one uses an interactive login telegraf seems to start up and run as console app. In this specific case, we are using a wrapper exe, the login then gets passed to telegraf. Because the login is non-interactive telegraf is attempting to run as a service and failing. Does that make sense?

@danielnelson
Copy link
Contributor

Got it. What do you think about the flag being -console? Also, can you make sure to test the signal handling as the reloadLoop branch was previously unix only. https://golang.org/pkg/os/signal/#hdr-Windows.

@aMaZing0x41
Copy link

I think the -console flag makes sense. We will take a look at the signal handling. Hopefully within the next week or so. Thanks for the feedback!

This was referenced May 18, 2017
Closed
Closed
@danielnelson danielnelson modified the milestones: 1.4.0, 1.5.0 Aug 14, 2017
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017
@danielnelson danielnelson mentioned this pull request Oct 26, 2017
Closed
@danielnelson danielnelson modified the milestones: 1.5.0, 1.6.0 Nov 30, 2017
@kerams kerams mentioned this pull request Dec 19, 2017
Closed
@kerams kerams mentioned this pull request Jan 15, 2018
Closed
@aMaZing0x41
Copy link

aMaZing0x41 commented Jan 19, 2018 •
edited
Loading

I have tested these changes on a recent version of master (5bac086). All appears well. @danielnelson mentioned the signaling. As far as I can see the exe is accepting signals. The process stops cleanly. You mentioned the reloadLoop. Is there anything else to test regarding that?

aMaZing0x41
aMaZing0x41 reviewed Jan 19, 2018
View reviewed changes
cmd/telegraf/telegraf.go Outdated
@@ -54,6 +54,7 @@ var fUsage = flag.String("usage", "",
"print usage for a plugin, ie, 'telegraf -usage mysql'")
var fService = flag.String("service", "",
"operate on the service")
var fRunAsConsole = flag.Bool("run-as-console", false, "run as console application (windows only)")

// Telegraf version, populated linker.
// ie, -ldflags "-X main.version=`git describe --always --tags`"
Copy link

Choose a reason for hiding this comment

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

Perhaps the usage string should be updated to contain the new console flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah just rename the flag and we can merge.

@aMaZing0x41
Copy link

@danielnelson Thanks. @MondayHopscotch updated the flag to console. The circleci tests are failing, but it doesn't appear to be related to this change. Is this a known issue? Please let us know if we need to update anything else.

@danielnelson danielnelson merged commit d831dbc into influxdata:master Jan 25, 2018
@danielnelson
Copy link
Contributor

danielnelson commented Jan 25, 2018 •
edited
Loading

Tests would pass if the branch was merged/rebased, looks like the failures are what needed fixed when we moved to Go 1.9.

@MondayHopscotch MondayHopscotch deleted the run-as-console branch January 26, 2018 14:48
maxunt pushed a commit that referenced this pull request Jun 26, 2018
@MondayHopscotch @danielnelson
Allow running as console application on Windows (#2754)
1e5ee78
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aMaZing0x41 aMaZing0x41 aMaZing0x41 left review comments

@danielnelson danielnelson danielnelson left review comments

Assignees
No one assigned
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin platform/windows
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@MondayHopscotch @aMaZing0x41 @danielnelson