-
Notifications
You must be signed in to change notification settings - Fork 27
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: load modules from multiple paths, support symlink as module #362
Conversation
- Resolve symlinks in modules directory, use symlink name to determine module name and order - Support multiple paths to search for modules. - Support directories without number prefix as module dirs. Default order is 1. - Remove '3 digits prefix' restriction. - Load values.yaml from all paths. Signed-off-by: Ivan Mikheykin <[email protected]>
Signed-off-by: Ivan Mikheykin <[email protected]>
Signed-off-by: Ivan Mikheykin <[email protected]>
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.
No real stoppers, but there are possible enhancements
**GLOBAL_HOOKS_DIR** — a directory with global hook files. | ||
|
||
**ADDON_OPERATOR_NAMESPACE** — a required parameter with namespace where Addon-operator is deployed. | ||
**MODULES_DIR** — paths separated by colon where modules are located. |
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.
MODULES_DIRS
would be better
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 also thought about MODULES_PATH
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.
MODULES_PATHS !
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.
This rename is a huge thing, it'll require changes in manifests, I don't want to make it in this PR.
RUNNING.md
Outdated
**ADDON_OPERATOR_NAMESPACE** — a required parameter with namespace where Addon-operator is deployed. | ||
**MODULES_DIR** — paths separated by colon where modules are located. | ||
|
||
**UNNUMBERED_MODULE_ORDER** — a default order for modules without numbered prefix. |
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.
What values can be here?
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.
Explained
} | ||
|
||
if name != "values.yaml" { | ||
log.Warnf("Ignore '%s' while searching for modules", absPath) |
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.
This function is going to return empty string, denoting this is not a directory. So this entry will be ignored. Why would we want this warning?
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.
Those warnings and errors are cheap substitutions for dry-run/lint mode and ignore file :(
pkg/module_manager/module_manager.go
Outdated
} | ||
log.Errorf("Possible bug!!! GetModule: no module '%s' in ModuleManager indexes", name) |
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.
It's not a bug, just no such module. It is fine, CALM DOWN AND DON'T PANIC!!!
deckhouse-controller module values my-inexisting-module
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 point, yeah. The caller should check for nil.
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.
it's more about the exclamation ""Possible bug!!!" :)
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.
Done
Co-authored-by: Eugene Shevchenko <[email protected]>
…explain data type for UNNUMBERED_MODULE_ORDER Signed-off-by: Ivan Mikheykin <[email protected]>
Overview
What this PR does / why we need it
Related issues:
Special notes for your reviewer
Does this PR introduce a user-facing change?