-
Notifications
You must be signed in to change notification settings - Fork 143
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
Revamp Sparkler Config - Type checking + Validation + *-site.yaml overriding #93
Comments
Reply from @SHASHANK-PRO-05 in IRDS email thread (Posting here for visibility)
|
Reply from @thammegowda in IRDS mailing list
|
Reply from @SHASHANK-PRO-05 in IRDS mailing list
|
Reply from @SHASHANK-PRO-05 in IRDS mailing list:
|
@SHASHANK-PRO-05 Sounds good to me.
I will be excited to see these changes coming! CC @karanjeets Check this out. |
@thammegowda @karanjeets Let me know if you find things that can be done better. https://github.com/SHASHANK-PRO-05/sparkler.git. I am doing development in "dev" branch. Also, I am creating reusable annotations in utils package. Do check that out also |
There are two FIXME: in configuration:
First, support loading
sparkler-defaults.yaml
andsparkler-site.yaml
. The common practice is*-default.yaml
provides default and recommended values from developers. The*-site.yaml
should beused by users to customize or to override the defaults. Right nowsparkler-default.yaml
is supported andsparkler-site.yaml
is not yet supported.Second, towards the stability of system and fail early when errors are introduced by humans.
Right now YAML configuration is transformed to JSON and internally it is treated as JSON. The system wouldn't know ahead of time if config is invalid. There is no type check for values, no schema validation etc. This could introduce errors to system and crash in later stages when the input config is invalid. As you see, except the config module, the rest of system is in staticly typed and strictly enforced. I think it will be nice to fail early when the config is invalid.
The text was updated successfully, but these errors were encountered: