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

FI-2532: Add feature flag to toggle using HL7 validator wrapper #497

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

dehall
Copy link
Contributor

@dehall dehall commented Feb 29, 2024

This PR adds a new feature flag to toggle between using the current Inferno validator wrapper and the HL7 validator wrapper. Set env var USE_HL7_RESOURCE_VALIDATOR = true to switch over to the HL7 validator, leave it blank/unset or set to anything else and it will use the current Inferno validator.

Summary of code changes

To make this a "one-click" toggle we'll need to have both validators enabled in the docker-compose file and nginx. With that in mind some of the changes here are just reverting changes from the WIP branch on #488 to re-enable the Inferno validator, so it may be useful to review the diff against main as well. If we're ok with the 3-step directions of "change env var, uncomment this service in docker-compose.yml, uncomment this section in nginx.conf" then we could leave the the HL7 validator entries commented out by default.

The real logic that handles the toggle is in onc_certification_g10_test_kit.rb - just a simple if so that the cliContext and igs settings aren't called for the inferno validator, and a conditional to call validator vs fhir_resource_validator.
I also noticed an instance where the capitalization of one error message made it slip through the filter so I updated that here as well.

Most of the remaining changes are for testing purposes. I didn't want to just skip tests depending on an env var, so I duplicated the resource_validation_spec.rb tests to run once for each type of validator. To make that work I needed a way to "reset" the validators so they reload appropriately per the env var. The simplest approach was to break the validator setup logic out into its own function that could be invoked later. Unfortunately rubocop checks cyclomatic complexity on method but not arbitrary blocks in the class definition so that didn't pass rubocop. Rather than a more significant refactor I just disabled that check in that one spot. If there's a better way to do this kind of "class reload" let me know. Then there's another test that I didn't know if it was worth a more significant refactor: bulk_data_group_export_validation_spec.rb, so this test just checks the current state of the env var and handles it appropriately. Let me know if this is worth digging more into.

Testing Guidance

No special guidance needed - as currently configured this should require only the one env var to be changed to toggle the desired result. You don't even need to restart services after toggling the option.

@dehall dehall marked this pull request as ready for review February 29, 2024 17:54
@Jammjammjamm
Copy link
Contributor

With the feature flag, I think we should leave all the config defaulting to the old validator. Stick the new env var in .env, set it to false, and put the additional instructions for enabling the new validator there in .env so you see it when you're trying to change the validator.

@dehall
Copy link
Contributor Author

dehall commented Mar 4, 2024

Updated - everything defaults to the old validator now. Again that makes the diff on this PR against the earlier "HL7 validator" pr look a little confusing, so looking at the diff with main may be helpful to confirm where things aren't really changing.

@dehall dehall merged commit 07fec59 into fi-2460-hl7-validator Mar 4, 2024
@dehall dehall deleted the fi-2532-validator-feature-flag branch March 4, 2024 18:53
dehall added a commit that referenced this pull request Mar 12, 2024
* FI-2532: add feature flag to switch between inferno validator and hl7 validator wrapper

* standardize name

* default hl7 validator to false and comment out its services
Jammjammjamm pushed a commit that referenced this pull request Apr 8, 2024
* FI-2460: Start adding support for HL7 validator wrapper

* Add cliContext with fields matching original validator settings

* switch dockerfile to use ubuntu jammy instead of alpine

* rework dockerfile to fetch release jar instead of completely rebuilding

* add new env vars for HL7 validator, restore env vars for inferno validator

* FI-2532: Add feature flag to toggle using HL7 validator wrapper (#497)

* FI-2532: add feature flag to switch between inferno validator and hl7 validator wrapper

* standardize name

* default hl7 validator to false and comment out its services

* use new inferno-resource-validator docker image, remove custom dockerfile

* Fix HL7 validator wrapper version at 1.0.51

* Set env var so validator sessions never expire
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