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

Replace FastAPI class with PHDI BaseService for container apps #520

Merged
merged 22 commits into from
May 8, 2023

Conversation

robertmitchellv
Copy link
Collaborator

PULL REQUEST

Summary

For most of the container apps' main.py there is boilerplate metadata being passed to FastAPI. In an effort to be more DRY, phdi.containers.base_service offers a BaseService class to handle the redundancies.

Related Issue

Fixes ? (not sure if there is already an issue for this)

@robertmitchellv
Copy link
Collaborator Author

Still working on tabulation and validation

@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

Merging #520 (353f09c) into main (cb21800) will not change coverage.
The diff coverage is n/a.

❗ Current head 353f09c differs from pull request most recent head 1e9f9f1. Consider uploading reports for the commit 1e9f9f1 to get more accurate results

@@           Coverage Diff           @@
##             main     #520   +/-   ##
=======================================
  Coverage   96.49%   96.49%           
=======================================
  Files          45       45           
  Lines        2479     2479           
=======================================
  Hits         2392     2392           
  Misses         87       87           
Flag Coverage Δ
unit-tests 96.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@robertmitchellv robertmitchellv marked this pull request as ready for review April 29, 2023 02:24
@robertmitchellv
Copy link
Collaborator Author

wrapped up everything (except FHIR server; have a question on that one for Dan when he's back)

Copy link
Collaborator

@emmastephenson emmastephenson left a comment

Choose a reason for hiding this comment

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

This looks great, Robert! Thanks!

Copy link
Collaborator

@DanPaseltiner DanPaseltiner left a comment

Choose a reason for hiding this comment

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

@robertmitchellv this is looking really great! Thanks for your work here. Before merging I think we can simplify some of our unit testing. The services that use the /health-check endpoint from base_service no longer need to have that endpoint covered by the service specific unit test since we have unit test coverage for this endpoint in phdi. Could these, unit tests that are now redundant be removed as part of this PR?

@robertmitchellv
Copy link
Collaborator Author

Thanks Dan; yes, absolutely, I can work on that this morning.

Copy link
Collaborator

@DanPaseltiner DanPaseltiner 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 removing those tests!

@robertmitchellv
Copy link
Collaborator Author

Tests are passing on local--I built the docker container for the building block locally to see what was going on--looks like a shell command is trying to remove something that isn't there:

Step 5/20 : RUN rm src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/TestData/DecompressedFiles/.wh.VXU_V04.liquid
 ---> Running in 09851ded28aa
rm: cannot remove 'src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/TestData/DecompressedFiles/.wh.VXU_V04.liquid': No such file or directory
The command '/bin/sh -c rm src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/TestData/DecompressedFiles/.wh.VXU_V04.liquid' returned a non-zero code: 1

Will work on fixing this error.

Copy link
Contributor

@BradySkylight BradySkylight left a comment

Choose a reason for hiding this comment

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

This looks great! I'm so glad we are doing this with all the containers. Excellent work Robert!

Copy link
Contributor

@BradySkylight BradySkylight left a comment

Choose a reason for hiding this comment

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

This looks great! I'm so glad we are doing this with all the containers. Excellent work Robert!

Copy link
Member

@nickclyde nickclyde left a comment

Choose a reason for hiding this comment

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

Great work Robert!!

@robertmitchellv robertmitchellv merged commit b902fca into main May 8, 2023
@robertmitchellv robertmitchellv deleted the robert/add-baseclass-to-containers branch May 8, 2023 20:29
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.

5 participants