Skip to content
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

Merged
merged 12 commits into from
Jul 18, 2023
Merged

Conversation

rohkhann
Copy link
Contributor

@rohkhann rohkhann commented Jul 17, 2023

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.

  • Also made some name simplifications in test files.

How was this tested?

  • Integration Tests
    yes
  • Unit Tests
    yes

Copy link
Contributor

@aaronpowell aaronpowell left a 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.

Copy link
Contributor

@aaronpowell aaronpowell left a 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.

Copy link
Contributor

@seantleonard seantleonard left a 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

Copy link
Contributor

@seantleonard seantleonard left a 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

@rohkhann rohkhann merged commit f260f28 into main Jul 18, 2023
@rohkhann rohkhann deleted the rohkhann/RunTimeConfigConstructor branch July 18, 2023 23:53
@Aniruddh25 Aniruddh25 changed the title Rohkhann/run time config constructor RuntimeConfig constructor Jul 21, 2023
@Aniruddh25 Aniruddh25 changed the title RuntimeConfig constructor Add FileSystemRuntimeConfigLoader Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Ability for uses to provide thier own implementation of runtimeconfig loader
3 participants