-
Notifications
You must be signed in to change notification settings - Fork 218
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
Add FileSystemRuntimeConfigLoader
#1587
Conversation
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.
Added a few notes around naming conventions.
While I've provided them as suggestions on the GitHub code review, it'd be better to do the refactor in VS as then it'll update all references.
Also, the file names will need to be updated to match the new type names.
fixing typo. Co-authored-by: Aaron Powell <me@aaron-powell.com>
…esystemruntimeconfigloader
…m/Azure/data-api-builder into rohkhann/RunTimeConfigConstructor
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.
Generally speaking this looks good.
Added a small nit in there and some notes on where we've got areas that need to be reworked in the future phases of this workstream.
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.
thanks for contributing these changes! just a couple comments
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.
Thanks for adding clarification to my questions
FileSystemRuntimeConfigLoader
Why make this change?
Different users of dataapi builder may have different ways in which they want to load in runtimeconfig. For example: There is a scenario where one might want to create the runtimeconfig object dynamically without having a runtime json ready.
What is this change?
In this pr, we create an abstract base class RunTimeConfigLoader that has certain common methods that all derived classes can use. The main TryLoadconfig method will be implemented differently by different classes. The RunTimeConfigProvider by dab loads config from the filesystem. Depending on use case this can be done differently by different consumers.
How was this tested?
yes
yes