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

Add context to Start, Init, Stop, and Finalize methods #5

Merged
merged 10 commits into from
Jan 1, 2021

Conversation

efritz
Copy link
Member

@efritz efritz commented Dec 21, 2020

Closes go-nacelle/nacelle#5.

Changes:

  • Start, Init, Stop, and Finalize methods now accept a context (supplied via Runner.Run)
  • Made Process optionally conform to Finalize interface
  • Context for Start is canceled after calling Stop (but before calling Finalize)

Copy link
Contributor

@aphistic aphistic left a comment

Choose a reason for hiding this comment

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

Would this be the appropriate PR to add Process and Init options for ProcessBaseContext and InitializerBaseContext? I envision those working similar to how http.Server.BaseContext works where you pass in a function of func(context.Context) context.Context to allow you to modify the context derived for each of the Process's functions. In addition to both ProcessBaseContext and InitializerBaseContext I thought that the bootstrap could have a WithBaseContext with a similar signature that would be run before all those other individual methods run. This way a user could choose to either have an overall modification for all the contexts, or just modify the one for their specific process or initializer.

@aphistic
Copy link
Contributor

We may also want to call out in the docs specifically which domains a context is associated with. If you have a timeout/deadline/etc set on a context, is that same context used for Init, Start and Stop, or is it applied to each of those individually?

@efritz
Copy link
Member Author

efritz commented Dec 30, 2020

Docs will be out of date as they're transferred to the new docsite and rewritten.

@efritz
Copy link
Member Author

efritz commented Dec 30, 2020

@aphistic Updated. Will still need to add a WithContext/WithContextFilter for the context passed to Run from the boostrapper.

@efritz efritz requested a review from aphistic December 30, 2020 21:15
@sonarcloud
Copy link

sonarcloud bot commented Dec 30, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@efritz efritz merged commit e29bf10 into master Jan 1, 2021
@efritz efritz deleted the rework-interfaces branch January 1, 2021 19:04
@aphistic
Copy link
Contributor

aphistic commented Jan 1, 2021

I started writing a comment awhile ago but I guess I never actually sent it. =/ I'm not sure I like the term "Filter" for the thing you use to modify the context because, to me, it implies you're removing something when it's actually the opposite in this case.

@efritz
Copy link
Member Author

efritz commented Jan 1, 2021

I started writing a comment awhile ago but I guess I never actually sent it. =/ I'm not sure I like the term "Filter" for the thing you use to modify the context because, to me, it implies you're removing something when it's actually the opposite in this case.

I'm not married to those names. I can update them globally before I tag v2.

Could you possibly make a list of other clunky names I could take care of around the same time?

@aphistic
Copy link
Contributor

aphistic commented Jan 2, 2021

Could you possibly make a list of other clunky names I could take care of around the same time?

Sure, do you know when you'll need that? I can make sure to have it reviewed by then.

@efritz
Copy link
Member Author

efritz commented Jan 2, 2021

Anytime before we cut the v2 branch since I'd want to get rid of the bad ones immediately.

@efritz
Copy link
Member Author

efritz commented Jan 2, 2021

Just mention them as you see them and I'll do a quick rename everywhere.

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.

Provide a way to use a context when running a process
2 participants