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-3259: Include presets in gems #572

Merged
merged 5 commits into from
Dec 13, 2024
Merged

Conversation

Jammjammjamm
Copy link
Collaborator

Summary

This branch updates the template to include presets in gems, and updates inferno to automatically load presets from gems. It also replaces any instances of https://inferno.healthit.gov with https://inferno-qa.healthit.gov when running on QA.

In order for the presets to be automatically loaded from gems, their gemspecs will have to be updated to include the preset files, as well as to set spec.metadata['inferno_test_kit'] = 'true', as shown in the template updates.

Testing Guidance

Add the following to the Gemfile:

gem 'onc_certification_g10_test_kit',
  git: 'https://github.com/onc-healthit/onc-certification-g10-test-kit.git',
  branch: 'fi-3259-demo'

Then follow the remaining instructions for running g10 from core in the Gemfile.

If you bundle exec inferno start and select the g10 suite, you will see the preset.

You can use the console to verify that the presets are altered when running on QA.

ᐅ be inferno c
...
[1] pry(main)> Inferno::Repositories::Presets.new.all.last
=> #<Inferno::Entities::Preset:0x000000012d69f888
 @id="us-core-7-reference-server",
 @inputs=
  [{:name=>"url", ..., :value=>"https://inferno.healthit.gov/reference-server/r4"},
  ...],
 @test_suite_id="us_core_v700",
 @title="Inferno Reference Server">

ᐅ INFERNO_HOST=https://inferno-qa.healthit.gov/suites be inferno c
...
[1] pry(main)> Inferno::Repositories::Presets.new.all.last
=> #<Inferno::Entities::Preset:0x000000013f45f840
 @id="us-core-7-reference-server",
 @inputs=
  [{:name=>"url", ..., :value=>"https://inferno-qa.healthit.gov/reference-server/r4"},
  ...],
 @test_suite_id="us_core_v700",
 @title="Inferno Reference Server">

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.17%. Comparing base (4034350) to head (39168ba).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/inferno/config/boot/presets.rb 85.71% 1 Missing ⚠️
lib/inferno/repositories/presets.rb 83.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #572   +/-   ##
=======================================
  Coverage   84.17%   84.17%           
=======================================
  Files         271      271           
  Lines       11581    11589    +8     
  Branches     1277     1279    +2     
=======================================
+ Hits         9748     9755    +7     
- Misses       1823     1824    +1     
  Partials       10       10           
Flag Coverage Δ
backend 92.57% <84.61%> (-0.01%) ⬇️
frontend 79.15% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Shaumik-Ashraf Shaumik-Ashraf left a comment

Choose a reason for hiding this comment

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

Followed testing guidance and confirmed presets loading in from test kit gem.

end

if Application['base_url'].start_with? 'https://inferno-qa.healthit.gov'
raw_contents.gsub!('https://inferno.healthit.gov', 'https://inferno-qa.healthit.gov')
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this opens the gateway to a lot of if Application['base_url'] ... statements. Inferno really needs some environment-based config built into the framework, but we already require .env files in test kits, so we can't use dotenv files to prepopulate a bunch of host-specific ENV[...] settings.

That's honestly an issue for another ticket though. Maybe test kit writers can be really good at putting all their environment-specific test kit things into a preset, and then we'll support something like ENV['INFERNO_ALLOWED_PRESETS'] = "preset_id1,preset_id2".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like this opens the gateway to a lot of if Application['base_url'] ... statements.

I don't know that I agree. This is to address a specific issue with presets.

  • Locally, you generally want the preset to point to prod. So test kits hard code presets pointing at inferno.healthit.gov, i.e. something different than INFERNO_HOST.
  • On prod, you want them to point to inferno.healthit.gov, i.e. the same thing as INFERNO_HOST
  • On QA, you want them to point to inferno-qa.healthit.gov, i.e. the same thing as INFERNO_HOST

Right now I handle this by altering all of the presets in the deployment repo so that they use erb to dynamically determine the correct host. We could add another environment variable in order to make things work correctly locally, in prod, and in QA, but that would require all of the presets that point to the reference server to be rewritten in every test kit. Or we could do this and let all of the existing presets work as they are everywhere, and let people continue to create them in a very straightforward way.

Inferno really needs some environment-based config built into the framework, but we already require .env files in test kits, so we can't use dotenv files to prepopulate a bunch of host-specific ENV[...] settings.

I'm not sure what you mean here. That is exactly what the .env files are for. But telling test authors that instead of just pointing their presets to https://inferno.healthit.gov/reference-server/r4, they need to turn their preset into an erb file and point things to <%= ENV.fetch('INFERNO_REFERENCE_SERVER_HOST') %>/reference-server/r4 or whatever is not great, and is the sort of thing people would mess up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this works for our purposes. I'm just thinking from the perspective of a generic framework. If someone wants to deploy a test kit to their production site and staging site but use the same preset file, they need to duplicate .env.production with the core settings we already mandate, add their own custom env variables, and have their ERB preset use those. AFAIK dotenv files aren't composable so it can get ugly to maintain. I realize this is a low-reward aspirational feature though, devs can just offer two presets with clearly labelled titles.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding of our design, hardcoding "inferno.healthit.gov" anywhere in Framework violates our design (forming a dependency from core to one platform's public hosting locations). I believe hardcoding "inferno.healthit.gov" in Test Kit presets violates our design too (forming a dependency from a test kit and one possible platform, though not quite as bad). Whether its worth the effort to decouple these for this type of thing... seems like Steve is saying 'no'.

@Jammjammjamm Did you consider putting this logic into inferno-deployment, so core doesn't have to know anything about those domains? As long as you considered that and decided it wasn't worth it, then I'm ok with this approach all things considered.

@Jammjammjamm Jammjammjamm force-pushed the fi-3259-include-presets-in-gems branch from ab9d85b to 39168ba Compare December 12, 2024 17:16
@Jammjammjamm Jammjammjamm merged commit 1565a94 into main Dec 13, 2024
9 of 10 checks passed
@Jammjammjamm Jammjammjamm deleted the fi-3259-include-presets-in-gems branch December 13, 2024 13:54
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.

4 participants