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.
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
📖Adding ComponentConfig proposal #818
📖Adding ComponentConfig proposal #818
Changes from all commits
23c65a4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI for backwards compatibility flags usually need to take precedence over config (unless you can force users to use exclusively flags or config).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout, since we're decoding in the same object that the flags could have already been set on there isn't a way I'm aware of to not override existing values while decoding. Have any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
save off the flags and apply them after the decode? (merge the options structs together more or less)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that probably makes the most sense. I personally don't think this needs to be documented in the design doc, do you?
Feeling okay with the rest of this @shawn-hurley ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of
ctrl.NewFromComponentConfig()
that returnsManager
, would it be reasonable to havectrl.NewOptionsFromComponentConfig()
that returnsmanager.Options
. And then let callers callmanager.New()
themselves so that they have more flexibility in updatingmanager.Options
after it has been populated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally a possibility. I'd originally started with that path and ended up combining the funcs to make the new API quicker to implement.
@shawn-hurley @mtaufen @DirectXMan12 @stealthybox Do you have a preference? The options would be:
ctrl.NewFromComponentConfig()
which takes theComponentConfig
type and returns actrl.Manager
.or
ctrl.NewOptionsFromComponentConfig()
which returnsmanager.Options
that implementation wise would be passed intomanager.New()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought would be for the ComponentConfig-populating function to accept a pointer to a
manager.Options
struct so that something like this would be possible:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to pass similar params into each func, EG parsing out the
ComponentConfig
type into the options we need to have thescheme
, configfilename
, the component config type and theOptions
struct.I think both are fair flows and we could achieve the same goals with a composite function that returns the manager or returns the options.
Does anyone have strong opinions against this? @stealthybox @johnsonj @joelanford @djzager @shawn-hurley @mtaufen