FI-2727 Inferno CI #167
FI-2727 Inferno CI #167
Conversation
|
@dehall I'm actually going to move enabling the Docker CI into another PR |
There was a problem hiding this comment.
I have a couple minor notes but the big one is the action is still failing. Is there no way to get it to ignore that specific failing test? I really don't want to merge a CI action that we know will error every time - it means we have to go inspect the logs on every pr
.github/workflows/inferno_ci.yml
Outdated
| - name: Point test kit to experimental branch | ||
| working-directory: ${{ github.workspace }}/onc-certification-g10-test-kit | ||
| run: | | ||
| echo "gem 'inferno_core', git: 'https://github.com/inferno-framework/inferno-core.git', branch: 'fi-3182-clean-diff'" >> Gemfile |
There was a problem hiding this comment.
Is it necessary to use this branch instead of main? Looks like the inferno execute feature has been merged, and I don't see any short IDs referenced below
There was a problem hiding this comment.
Yes for now because that branch also includes a bug fix. Ideally this should point to the latest RubyGem release of inferno_core by the time it merges.
.github/workflows/inferno_ci.yml
Outdated
| workflow_dispatch: | ||
|
|
||
| env: | ||
| INFERNO_G10_RELEASE: v6.0.3 |
There was a problem hiding this comment.
I'm not a huge fan of manually specifying a tag here, it means every g10 release we'll have to remember to update this then make a PR, etc. If there's a way to get the latest tag that's probably better
There was a problem hiding this comment.
I think generally people would want their CI tied to a specific release instead of latest, but I'll investigate running against latest as well as fixed releases.
.github/workflows/inferno_ci.yml
Outdated
| run: | | ||
| docker compose build | ||
| docker compose up -d | ||
| wget -qO- https://raw.githubusercontent.com/eficode/wait-for/v2.2.3/wait-for | sh -s -- localhost:8080 -- echo "Server ready" |
There was a problem hiding this comment.
Piping a raw wget into a shell is a little scary but ok. Is the "wait-for-it" stuff in the repo not used anymore? Or could the script you're wgetting here be copied into the repo?
There was a problem hiding this comment.
Yes, wait-for-it doesn't work for our use case so I had to use wait-for. I'm thinking we'll either fork the wait-for repo, or copy in the script, or port the functionality into Ruby... still TBD.
| smart_app_launch_version:smart_app_launch_1 \ | ||
| multi_patient_version:multi_patient_api_stu1 \ | ||
| --groups 4 \ | ||
| --inputs "url:localhost:8080/reference-server/r4" \ |
There was a problem hiding this comment.
These inputs are a little hard to follow. Is it possible to save these off to a separate file and read them in here? (The web UI allows you to download a json or yml of test run inputs, can those be loaded here?)
There was a problem hiding this comment.
Yeah input preset file support is coming soon.
Agreed; I marked this as "ready" because the action is working as intended, but I wouldn't merge this into main. Unfortunately the way g10 is currently designed I'm unable to exclude just the TLS test that's causing this to fail. Currently I'm thinking this PR will get replaced by a GitHub Actions testing US Core instead of g10. I appreciate the feedback though! It helps me make a proper Github Action for this. |
I assume this is becuase you are passing an input to a test that doesn't exist? Should this just filter inputs passed to a test based on what the test inputs are? Inferno core should expose that pretty handily I'd hope? |
|
Yeah there was a bug that required me to do that originally, but it got patched in: inferno-framework/inferno-core#542 I'm going to soft expect users to not put extraneous inputs when they select specific groups/tests to run. |
|
Closing because PR #182 fulfills this task |
Summary
This PR is a draft for testing GitHub Actions... will update this when its ready for review.A full
inferno executeexample in GitHub Actions CI. Ready for early feedback.This action fails correctly because of the TLS test. See results here