Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

FI-928: Integrate generation with validator #476

Merged
merged 6 commits into from
Aug 6, 2020

Conversation

Jammjammjamm
Copy link
Contributor

This branch integrates the generic generator with the validator service. When Inferno starts up, the resources for any module created via the generic will be posted to the validator service so that it can validate resources. This only works for the contents of an IG package containing the corresponding package.json. If no package.json is present, the resources are not uploaded to the validator.

This functionality depends on inferno-framework/fhir-validator-wrapper#16 and inferno-framework/fhir-validator-wrapper#18

Loading inferno's modules has been moved from Inferno::Module to a new Inferno::StartupTasks. Loading modules now may involve making a POST to an external service, so this change makes it so that you won't make external HTTP requests just by requiring lib/app/models/module.rb.

Submitter:

  • This pull request describes why these changes were made
  • Internal ticket links to this PR
  • Internal ticket is properly labeled (Community/Program)
  • Internal ticket has a justification for its Community/Program label
  • Code diff has been reviewed for extraneous/missing code
  • Tests are included and test edge cases
  • Tests/code quality metrics have been run locally and pass

Reviewer 1:

Name:

  • Code is maintainable and reusable, reuses existing code and infrastructure
    where appropriate, and accomplishes the task's purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

Reviewer 2:

Name:

  • Code is maintainable and reusable, reuses existing code and infrastructure
    where appropriate, and accomplishes the task's purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

@Jammjammjamm Jammjammjamm self-assigned this Jul 31, 2020

begin
Inferno.logger.info "Posting IG for '#{module_name}' to validator"
RestClient.post("#{validator_url}/igs", File.read(file.path))
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can tell, you are not using the response from this route for anything, right? I just wanted to make sure because in inferno-community/fhir-validator-wrapper#16, we recently changed the response for the PUT and POST /igs routes. Just wanted to give you a heads-up in case this might break some functionality in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it receives a 4xx/5xx response, then an error is raised, otherwise it's not doing anything with the response.

@radamson radamson requested a review from arscan August 5, 2020 11:12
@radamson
Copy link
Contributor

radamson commented Aug 5, 2020

I think it'd be worth updating the docker-compose.yml because fhir-validator-service now has the package loading functionality included.

i.e.

  validator_service:
    image: infernocommunity/fhir-validator-wrapper

can be changed to

  validator_service:
    image: infernocommunity/fhir-validator-service

Copy link
Contributor

@arscan arscan left a comment

Choose a reason for hiding this comment

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

I like how this waits for the validator service to be up. I'm a little wary about not having any protection from simply overwriting our public service's IGs (could have some secret in an env variable, or blocking the route externally in our site-overlay). But that doesn't seem like a likely issue and we can wait on that. I wonder if this slows down startup time in practice... but if it does, we can address then.

Splitting these pieces off into microservices sure does add a lot of code. My dream is to reunify validation back into Ruby so we can get back to monolith. But this looks good for now 👍

Copy link
Contributor

@radamson radamson left a comment

Choose a reason for hiding this comment

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

Tested it again and it worked well. Still some issues when the IGs do not include a index.json, but that's more of an issue on the validator side.

@radamson radamson merged commit d08a5cf into development Aug 6, 2020
@radamson radamson deleted the fi-928-integrate-generation-with-validator branch August 6, 2020 16:44
@okeefm okeefm mentioned this pull request Aug 19, 2020
14 tasks
@radamson radamson mentioned this pull request Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants