microsoft/healthcare-shared-components

Refactor `SchemaManager`

jovinson-ms opened this issue · 7 comments

jovinson-ms commented

The SchemaManager tool under \tools could be improved in a few ways:

  • The namespace should begin with Microsoft.Health, not SchemaManager
  • There are SchemaManager and SchemaManager.Core projects that aren't used for different purposes
  • Dependencies are manually registered in Program.cs rather than using the registration methods in Microsoft.Health.SqlServer.Registration.
  • SchemaManager dependency registration isn't tested
rahul-s-bhatt commented

Hey, I would like to contribute to this issue.

jovinson-ms commented

Great! Why don't you take a look at the code, and then bullet point the changes you plan to make and I'll be happy to provide feedback.

rahul-s-bhatt commented

Thank you for giving me this opportunity, I'd plan to make several changes to the above issue mentioned in the following way:

  1. Change the namespace of the SchemaManager library under \tools along with the namespaces inside the files to Microsoft.Health.SchemaManager.
  2. Similar change of the namespace for SchemaManager.Core library under \tools along with the namespaces inside the files to Microsoft.Health.SchemaManager.Core.
  3. Refactor the Dependency Resolving code under \tools\SchemaManager\Program.cs similar to that of Microsoft.Health.SqlServer\Registration\SqlServerBaseRegistrationExtensions.cs.
jovinson-ms commented

Looks good - I would add a fourth step to run an integration test on the tool to ensure that when we run a command, the behavior is correct.

rahul-s-bhatt commented

@jovinson-ms I m sorry, I have no experience in testing yet, if you could help me with it, I'll do it.

wbalbo commented

@jovinson-ms I m sorry, I have no experience in testing yet, if you could help me with it, I'll do it.

Hey @rahulbhatt1899, you can see some examples here, in this project:

https://github.com/microsoft/healthcare-shared-components/blob/master/test/Microsoft.Health.SqlServer.Tests.Integration/Features/Schema/SchemaInitializerTests.cs

A very good tutorial about xUnit tests here: https://www.youtube.com/watch?v=ub3P8c87cwk

Is not hard, I learned recently, just follow the examples, create a test project first and learn a bit, then make your changes in your branch, push here and let us know.

PS: I'm not affiliated with this project, just another contributor trying to help, good luck.

jovinson-ms commented

Should have closed this earlier - we had to pause this while some urgent changes went in, and there's now an effort underway to refactor. Thanks all for the input!

  • 👍1