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

Separate config validation from FileSystemConfigPersistence #4169

Merged
merged 4 commits into from Jul 1, 2021

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Jun 16, 2021

What

  • When we started working on a PostgresConfigPersistence we noticed that were were duplicate the code for validating the schema of configs.

How

  • Instead of doing that duplication, this PR creates a new class ValidatingConfigPersistence that will decorate any ConfigPersistence with the validation logic.
  • Rename DefaultConfigPersistence => FileSystemConfigPersistence (in preparation of us having a PostgresConfigPersistence
  • Change usages in ServerApp and SchedulerApp to use the decorated-with-validation version of FileSystemConfigPersistence.
  • Hopefully this makes the next step of adding PostgresConfigPersistence a bit easier.

Other...

Instead of going with the decorator pattern here we could have these class extend a ValidatingConfigPersistence base instead. I'm open to that as well. In watching @subodh1810 deal with some of the validation stuff getting in the way that being able to interact with these persistences without opinionated validation might be helpful. I might be misguided on this though.

Recommended reading order

  1. FileSystemConfigPersistence.java
  2. ValidatingConfigPersistence.java
  3. the rest...

@tuliren tagged you since this will interact with the issue you are going to work in.

@github-actions github-actions bot added area/connectors Connector related issues area/platform issues related to the platform labels Jun 16, 2021
Copy link
Contributor

@tuliren tuliren 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 tagging me on this.

In watching @subodh1810 deal with some of the validation stuff getting in the way that being able to interact with these persistences without opinionated validation might be helpful.

Yes, I think that we can sometimes mess up the migration such that user's old config becomes invalid. So a pure persistence may be helpful in some cases. But that should be an easy change.

@cgardens
Copy link
Contributor Author

Will merge this this afternoon. Need to get one more test passing.

RunMigrationTest > testRunMigration() FAILED

@cgardens cgardens force-pushed the cgardens/move_config_persistence_validation branch from 8a840b9 to e4674ea Compare June 30, 2021 17:53
@github-actions github-actions bot removed the area/connectors Connector related issues label Jun 30, 2021
@cgardens cgardens merged commit d78a985 into master Jul 1, 2021
@cgardens cgardens deleted the cgardens/move_config_persistence_validation branch July 1, 2021 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants