Added ctx object to all system struct's methods #836
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The idea behind this commit is to set the stage for the following features: * logging - by adding context Values for logging purposes * timeout/cancellation - move this up the stack and slowly add support for all resources where this makes sense
3 tasks
|
went with #837 instead |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The idea behind this commit is to set the stage for the following features:
Checklist
make test-all(UNIX) passes. CI will also test thisDescription of change
Busy PR, lots of boring work to wire this up.. but hopefully this opens the door for some future features/flexibility.
One thing I wasn't 100% sure on is whether to have Validate() take ctx, or it instantiates it based on the yaml config.
My high level thoughts are:
Validate()- creates a new context and sets the timeout based on the value in yaml (or default)ValidateValue(..)- adds some values to the ctx object for logging purposescommand.Stdout(ctx)- thesystem/struct will then be able to log with the values from ValidateValue, and will timeout when the context is cancelled.Possible alternatives
Pass ctx only to the constructors, that would bring a lot of simplicity to the codebase as most of it would remain unchanged. Also, don't think any of the system resources have methods that require different timeouts.
I'll create another simpler PR with that approach and we can discuss how they compare.