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

feat: Add Windows Service support #1696

Merged
merged 9 commits into from
Apr 19, 2023
Merged

feat: Add Windows Service support #1696

merged 9 commits into from
Apr 19, 2023

Conversation

fkollmann
Copy link
Contributor

This PR adds support for running the Cloud SQL Proxy as Windows service.

More details on how to install, can be found in the windows-service-guide.md file.

Tested on Windows Server 2019.

If the executable is run from the command line directly, it will run normally like before.

Please see it as a first draft based on my multi-year experience with running Go apps as Windows services.

Open questions are:

  • Do we need an installer? (WiX, Nullsoft)
  • Should the path of the logfile be changeable?
  • Should the Windows Event Log be used for logging (requires installer to be feasible)?

Implements #277

@fkollmann fkollmann requested a review from a team as a code owner March 12, 2023 13:06
@fkollmann fkollmann changed the title Add Windows Service support feat: Add Windows Service support Mar 12, 2023
@conventional-commit-lint-gcf
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@enocom
Copy link
Member

enocom commented Mar 13, 2023

Wow! Thanks for this PR! I'll take a look today.

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

Generally looks good to me.

Tests would be nice ;-)

A few questions and minor suggestions.

main_windows.go Show resolved Hide resolved
windows-service-guide.md Outdated Show resolved Hide resolved
windows-service-guide.md Outdated Show resolved Hide resolved
main_windows.go Outdated Show resolved Hide resolved
main_windows.go Show resolved Hide resolved
@enocom enocom added the tests: run Label to trigger Github Action tests. label Mar 13, 2023
@github-actions github-actions bot removed the tests: run Label to trigger Github Action tests. label Mar 13, 2023
@enocom
Copy link
Member

enocom commented Mar 13, 2023

And to answer you questions:

Do we need an installer? (WiX, Nullsoft)

For now, installing from the Releases page seems fine to me.

Should the path of the logfile be changeable?

This is also easy to change in the future, so I'm inclined to start with a hard-coded path and update if there's interest.

Should the Windows Event Log be used for logging (requires installer to be feasible)?

Might be another nice enhancement, but we can ship this once we work through review. So I'm ok with not doing this yet.

@enocom
Copy link
Member

enocom commented Mar 20, 2023

@fkollmann Friendly ping.

If you'd like some help getting this finished, let me know. You've already done the majority of the work. 😄

@fkollmann
Copy link
Contributor Author

@fkollmann Friendly ping.

Thank you. Sry, I was busy playing Diablo IV beta this weekend 😅. I will get back to you on Thursday.

@enocom
Copy link
Member

enocom commented Mar 20, 2023

😆 No worries. Again this is a helpful contribution, so I'm happy to lend a hand wherever needed.

@fkollmann
Copy link
Contributor Author

I went through the comments and see no major issues, besides a minor misunderstanding on Windows services. I will answer the comments tomorrow and make some changes.

@fkollmann
Copy link
Contributor Author

Short update: I had to change plans for the weekend. I have time for providing feedback today.

@fkollmann
Copy link
Contributor Author

I did some code changes and marked the suggestions as resolved. (Please let me know if I should have kept the suggestions unresolved, or not).

Looking forward to your feedback.

@enocom
Copy link
Member

enocom commented Mar 30, 2023

Sorry for the slow response. I'll get to this soon.

@fkollmann
Copy link
Contributor Author

Don't worry. No pressure from my side.

We are currently using it on production, so it at least works for us 😅

@enocom enocom added the tests: run Label to trigger Github Action tests. label Apr 3, 2023
@github-actions github-actions bot removed the tests: run Label to trigger Github Action tests. label Apr 3, 2023
@enocom
Copy link
Member

enocom commented Apr 4, 2023

Currently running this through some manual tests. We can probably get this into the next release.

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

Looks great. Just a few comments.

Do we need to add a note about UAC as a possible thing to investigate if it doesn't work?

windows_install_service.bat Show resolved Hide resolved
windows-service-guide.md Outdated Show resolved Hide resolved
windows-service-guide.md Show resolved Hide resolved
@enocom
Copy link
Member

enocom commented Apr 17, 2023

@fkollmann There are a few small changes to make here that I'm going to do. And then I'll merge this.

@enocom
Copy link
Member

enocom commented Apr 17, 2023

Looks like I can't push to your fork. You can change that, though. See here.

Alternatively, aside from the two typos @jackwotherspoon pointed out, we need to keep a main.go and just add a //go:build !windows at the top, so we can keep building for Unix-based OSes.

@fkollmann
Copy link
Contributor Author

Hi, sry, I was on vacation the last week, incl today. I will look into this tomorrow. Have a nice evening :) . Best Regards, Felix

@fkollmann
Copy link
Contributor Author

Looks like I can't push to your fork. You can change that, though. See here.

That is currently not support with forks below other organizations (instead of individual users). However, I granted you maintainer permissions on the repo.

@fkollmann
Copy link
Contributor Author

Okay, I addressed everything, from my POV. Please feel free to comment (and change). @enocom

@enocom enocom added the tests: run Label to trigger Github Action tests. label Apr 19, 2023
@enocom
Copy link
Member

enocom commented Apr 19, 2023

Looks great! Thanks again for this contribution!

@github-actions github-actions bot removed the tests: run Label to trigger Github Action tests. label Apr 19, 2023
@enocom enocom merged commit ec56eba into GoogleCloudPlatform:main Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants