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(svc): create Run method to encapsulate most of whats in func main #533

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

AndrewWinterman
Copy link
Contributor

Please read CONTRIBUTING.md for additional information on contributing to this repository!

What this PR does / why we need it

This PR extracts most of the main function provided by stencil-golang into a
function clients can call instead.

QSS runs a number of peripheral services-- CDC, Ingest, Polaroid, SmartWorker,
etc., each of which need to set up their own main function. To do this, they
basically copy whats in stencil-golang. Wouldn't it be better if we could use a
library function instead? I think it would be, since it eases the burden when
it comes time to update any service-wide behavior.

Of course, we'll still want stencil to provide the kubernetes objects, e.g.
trace.yaml for example, and my hope is we'll stencil in depending on this
method instead of just doing what it does currently inline.

Jira ID

[XX-XX]

Notes for your reviewers

This PR extracts most of the main function provided by stencil-golang into a
function clients can call instead.

QSS runs a number of peripheral services-- CDC, Ingest, Polaroid, SmartWorker,
etc., each of which need to set up their own main function. To do this, they
basically copy whats in stencil-golang. Wouldn't it be better if we could use a
library function instead? I think it would be, since it eases the burden when
it comes time to update any service-wide behavior.

Of course, we'll still want stencil to provide the kubernetes objects, e.g.
trace.yaml for example, and my hope is we'll stencil in depending on this
method instead of just doing what it does currently inline.
@AndrewWinterman AndrewWinterman requested a review from a team as a code owner August 8, 2024 23:47
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Shouldn't templates/cmd/main.go.tpl also be changed in this PR?

pkg/go.mod Outdated Show resolved Hide resolved
pkg/run/run.go Show resolved Hide resolved
pkg/run/run.go Outdated
Comment on lines 5 to 9
// Package run provides a function that can be invoked inside of main to set up
// all the service-standard components we expect every service to run.
//
// clients should provide any runners they require as part of the app to the
// Run function with OptAddRunner, and then call `Run` in their main function.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: it might be worth adding an example of how to use this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a testable example... should probably add a note that it exists here.

Copy link
Contributor

@george-e-shaw-iv george-e-shaw-iv left a comment

Choose a reason for hiding this comment

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

I've already reviewed the majority of this code. I think Mark has some fair points though

@AndrewWinterman
Copy link
Contributor Author

Shouldn't templates/cmd/main.go.tpl also be changed in this PR?

I had thought not, I'd like to merge it, use it some test services, and then circle back.

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