-
Notifications
You must be signed in to change notification settings - Fork 437
Add ability to opt-out of drop-in files #1331
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
Conversation
Pull Request Test Coverage Report for Build 18199556959Details
💛 - Coveralls |
8703143 to
51d85df
Compare
|
@cdesiniotis note that we explicitly distinguish between an empty value for Update: While adding a unit test to validate this behaviour, I noticed that this was not happening. I would still like to avoid adding an extra config option though. I have updated the PR. Let me know what you think. |
elezar
left a comment
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.
See my comment regarding RUNTIME_DROP_IN_CONFIG.
51d85df to
3449222
Compare
3449222 to
69d053c
Compare
elezar
left a comment
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.
Feel free to squash my commit on top and merge.
ed4a363 to
2667197
Compare
This change allows users to opt-out of using drop-in config files when configuring containerd or crio. This is useful in cases where the top-level config is a templated config file. Signed-off-by: Christopher Desiniotis <[email protected]> Co-authored-by: Evan Lezar <[email protected]>
2667197 to
3a59d9d
Compare
| containerd.WithRuntimeType(co.runtimeType), | ||
| containerd.WithUseLegacyConfig(co.useLegacyConfig), | ||
| containerd.WithContainerAnnotations(co.containerAnnotationsFromCDIPrefixes()...), | ||
| containerd.WithDisableDropInConfig(o.DropInConfig == ""), |
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.
I don't quite understand the benefit of adding a new field here, if we can just directly evaluate this conditional internally when processing the runtime config
o.DropInConfig == ""
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.
The DropInConfig path is not passed to the configs themselves.
|
After comment #1251 (comment) , do we still want this patch? |
No, I think we can hold off on this. Closing for now and we can revisit this if the need to disable drop-in files resurfaces. |
This change allows the use of drop-in config files for containerd and cri-o to be disabled when setting
RUNTIME_DROP_IN_CONFIGto"". This allows better support for use cases such asmicrok8swhere the top-level config may be a template that is rendered at runtime.