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

Orchestration service happy path #829

Merged
merged 75 commits into from
Oct 5, 2023
Merged

Orchestration service happy path #829

merged 75 commits into from
Oct 5, 2023

Conversation

gordonfarrell
Copy link
Collaborator

PULL REQUEST

Summary

Howdy howdy howdy.
Here's orchestration running on a default config and hitting Validation, FHIR Converter, Ingestion (Standardizing DOB, Phone, Name, and Geocoding), and the message parser that Nick and I wrote up.

This is happy path, not much in the way of accounting for individual step failures etc. This is because we wanted to check with the team on what exactly we want to happen on step failures and ALSO because the fix could either be handled here or there are some updates and optimizations that could be done on the BB endpoints to mitigate issues, e.g. making sure the bundle sent is always returned even on a failure.

Looking forward to feedback, cheers!

Ticket

https://app.zenhub.com/workspaces/secret-6480bf2ee530095ab41ebbe9/issues/gh/cdcgov/phdi/785

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 awesome - thank you both so much! 🙌

I left a couple of preliminary comments but overall this seems like the right direction to me 👍

containers/orchestration/app/main.py Outdated Show resolved Hide resolved
containers/orchestration/app/main.py Show resolved Hide resolved
containers/orchestration/app/main.py Show resolved Hide resolved
containers/orchestration/app/main.py Show resolved Hide resolved
containers/orchestration/app/utils.py Outdated Show resolved Hide resolved
containers/orchestration/local-dev.sh Outdated Show resolved Hide resolved
containers/orchestration/app/main.py Outdated Show resolved Hide resolved
containers/orchestration/app/main.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@JNygaard-Skylight JNygaard-Skylight left a comment

Choose a reason for hiding this comment

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

Looks good to me. The only minor thing is that some of the error messages do not return a lot of information, but that's it.

containers/orchestration/app/main.py Outdated Show resolved Hide resolved
containers/orchestration/app/main.py Outdated Show resolved Hide resolved
containers/orchestration/app/main.py Outdated Show resolved Hide resolved
}


def validation_payload(**kwargs) -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dumb question, but what are these payload definitions used for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

They define how each service needs it request defined. Some of them have different requirements, so we define each

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.

A couple of comments but nothing blocking - feel free to merge and address these later.

THANK YOU both for putting so much time in on this PR, exciting to see it at the finish line! 🏁 🎉

"ingestion": {
"url": "http://localhost:8083"
},
"standardization_and_geocoding": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should standardization & geocoding be sub-fields under ingestion? That seems to be the standard with the other BBs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd maybe have a preference to just read the sample configuration in the tests instead of keeping a second tests config - I think it would be easy for them to come out of sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point, but the one issue is that the test config drops geocoding so we don't have to worry about smarty creds/geocode failures. We'd have to come up with another way of dodging that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh gotcha. Could you make a ticket for this so we remember to follow up on it?

containers/orchestration/app/services.py Outdated Show resolved Hide resolved

def unzip(zipped_file) -> Dict:
my_zipfile = ZipFile(zipped_file.file)
file_to_open = [file for file in my_zipfile.namelist() if "/CDA_eICR.xml" in file][
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be for a followup PR, but I think we also need to look for the RR file here and pass that to services as appropriate.

Copy link
Collaborator

@KennethSkylight KennethSkylight left a comment

Choose a reason for hiding this comment

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

Lets get this in!!!

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.

:shipit:

@nickbristow nickbristow merged commit fe65e68 into main Oct 5, 2023
30 checks passed
@nickbristow nickbristow deleted the nick/gordon/config branch October 5, 2023 17:57
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