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.

Sign up for GitHub

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

Jump to bottom

Blob Storage and DICOM Blob Data Store #4

Merged
merged 10 commits into from Jun 5, 2019
Merged

Blob Storage and DICOM Blob Data Store #4

merged 10 commits into from Jun 5, 2019

Conversation

Richyl2
Copy link
Contributor

@Richyl2 Richyl2 commented May 31, 2019

Description

This PR is to add the blob storage capabilities to the solution. This work is currently not connected up to the DICOM web API project yet.

  • Added the base health projects for instantiating a blob storage provider
  • Added the DICOM blob data store interface and implementation that will be used for reading/ writing/ and storing DICOM files
  • Added tests projects for each new project, and added the integration test project framework

Testing

  • Added unit test project testing the health check feature and validating the blob client/ containers are created correctly
  • Added the integration project to test the DICOM blob data store.

@Richyl2
Added the blob storage provider base projects and DICOM storage proje…
e4902a0 
…ct. Also added framework for integration tests
@Richyl2 Richyl2 added the Area-DicomStore STOW-RS label May 31, 2019
@Richyl2 Richyl2 marked this pull request as ready for review May 31, 2019 15:10
@Richyl2 Richyl2 requested a review from a team as a code owner May 31, 2019 15:10
@Richyl2
Copy link
Contributor Author

Richyl2 commented Jun 3, 2019

@hansenms or @brendankowitz please review when you get the chance; thanks!

@hansenms
Copy link

hansenms commented Jun 3, 2019

This looks good to me in terms of overall structure, etc. I will let @brendankowitz provide comments on any minor details and approve PR when ready.

One question: Do we need to do the health tests in the initializers when they are done in the health check? Seems redundant?

brendankowitz
brendankowitz approved these changes Jun 4, 2019
View reviewed changes
Copy link
Member

@brendankowitz brendankowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me

src/Microsoft.Health.Blob/Registration/BlobClientRegistrationExtensions.cs Outdated Show resolved Hide resolved
src/Microsoft.Health.Blob/Registration/BlobClientRegistrationExtensions.cs Outdated Show resolved Hide resolved
@Richyl2
Removed the IScoping to the cloud blob client and updated the registr…
419b53e 
…ation of the cloud blob client to a singleton
@Richyl2 Richyl2 merged commit 2040989 into master Jun 5, 2019
@Richyl2 Richyl2 deleted the personal/richyl2/blob-storage-provider branch July 22, 2019 10:04
@npagare npagare mentioned this pull request Mar 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendankowitz brendankowitz brendankowitz approved these changes

@javier-alvarez javier-alvarez javier-alvarez left review comments

Assignees
No one assigned
Labels
Area-DicomStore STOW-RS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Richyl2 @hansenms @brendankowitz @javier-alvarez