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 additional param to the BaseService class to allow for passing other licenses to FastAPI #526

Merged
merged 9 commits into from
May 5, 2023

Conversation

robertmitchellv
Copy link
Collaborator

PULL REQUEST

Summary

Added a license_info param that is a dict with a default value that matches all DIBBs defaults for the container apps in phdi/containers. Given the fhir-converter is pulling a Microsoft base image in its Dockerfile and using Micosoft's FHIR Converter we should make a best effort to pass its MIT license and add it to the description.md for the fhir-converter, which is already done in #520

Related Issue

Fixes #520

…ed a test to pass a different license as a dict and tests pass locally
@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #526 (d9300da) into main (4fd35ac) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #526   +/-   ##
=======================================
  Coverage   96.48%   96.49%           
=======================================
  Files          45       45           
  Lines        2473     2479    +6     
=======================================
+ Hits         2386     2392    +6     
  Misses         87       87           
Flag Coverage Δ
unit-tests 96.49% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
phdi/containers/base_service.py 100.00% <100.00%> (ø)

@robertmitchellv robertmitchellv marked this pull request as ready for review May 3, 2023 21:48
@robertmitchellv
Copy link
Collaborator Author

This one is wrapped up and ready now--didn't notice the changes from #517 with the inclusion of from importlib import metadata and the version now coming from matadata.version("phdi"); fixed those conflicts and we're back to all green on tests

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.

LGTM. I think it would be nice if we could address that default_app_license is defined twice. This isn't a huge deal though so I'm not going to block merging.

Comment on lines 12 to 15
default_app_license = {
"name": "Creative Commons Zero v1.0 Universal",
"url": "https://creativecommons.org/publicdomain/zero/1.0/",
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this dictionary is also defined in base_service.py. Could we DRY this out a bit?

@robertmitchellv robertmitchellv merged commit 6632a53 into main May 5, 2023
@robertmitchellv robertmitchellv deleted the robert/add-license-param-baseservice branch May 5, 2023 22:55
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.

2 participants