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

15223: generate empty in convert #16137

Merged
merged 9 commits into from
Oct 8, 2024

Conversation

mkalish
Copy link
Collaborator

@mkalish mkalish commented Oct 7, 2024

This PR updates the code so that when an empty report (or any situation where the number of items cannot be determined) makes it to the convert step.

Test Steps:
There are a integration tests that covers this, but as the pipeline is currently structured this is hard to test manually as the report function will synchronously fail.

Changes

  • Adds a new trackEmptyReport that can be used anywhere in the pipeline
  • Updates the convert step to use it if it cannot determine the number of items

Checklist

Testing

  • Tested locally?
  • Ran ./prime test or ./gradlew testSmoke against local Docker ReportStream container?
  • (For Changes to /frontend-react/...) Ran npm run lint:write?
  • Added tests?

Process

  • Are there licensing issues with any new dependencies introduced?
  • Includes a summary of what a code reviewer should test/verify?
  • Updated the release notes?
  • Database changes are submitted as a separate PR?
  • DevOps team has been notified if PR requires ops support?

Linked Issues

To Be Done

Specific Security-related subjects a reviewer should pay specific attention to

@mkalish mkalish added the platform Platform Team label Oct 7, 2024
Copy link

github-actions bot commented Oct 7, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

@mkalish mkalish changed the title Platform/kalish/15223 generate empty in convert 15223: generate empty in convert Oct 7, 2024
Copy link

github-actions bot commented Oct 7, 2024

Test Results

1 239 tests  +2   1 235 ✅ +2   7m 47s ⏱️ +22s
  162 suites ±0       4 💤 ±0 
  162 files   ±0       0 ❌ ±0 

Results for commit 42e5870. ± Comparison against base commit 1333c99.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Oct 7, 2024

Integration Test Results

 53 files   53 suites   27m 38s ⏱️
410 tests 401 ✅ 9 💤 0 ❌
413 runs  404 ✅ 9 💤 0 ❌

Results for commit 42e5870.

♻️ This comment has been updated with latest results.

@mkalish mkalish marked this pull request as ready for review October 7, 2024 20:36
@mkalish mkalish requested a review from a team as a code owner October 7, 2024 20:36
Copy link
Collaborator

@thetaurean thetaurean left a comment

Choose a reason for hiding this comment

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

LGTM 👍 One small comment:

I wonder if there's any tech debt in ActionHistory.kt worth ticketing. I remember getting a little lost / confused when tracking a created empty report in other steps.

Separate but related, should we scope some tickets for using this new function in other places we generate an empty report (e.g. FHIRDestinationFilter.kt and FHIRReceiverFilter.kt)? Will spend a little more time thinking this over tomorrow.

@mkalish
Copy link
Collaborator Author

mkalish commented Oct 8, 2024

@thetaurean 100% think there are tech debt tickets to be made for action history but I have always been unsure where to start since I think there could be an argument to simply rewrite the whole thing.

For the other pipeline steps, maybe? This spot in in convert is a little unique in the sense that its only part of the pipeline where the number of items is unknown in case of an error

@mkalish mkalish enabled auto-merge (squash) October 8, 2024 14:32
Copy link

sonarcloud bot commented Oct 8, 2024

@mkalish mkalish merged commit 5025bf5 into master Oct 8, 2024
22 checks passed
@mkalish mkalish deleted the platform/kalish/15223-generate-empty-in-convert branch October 8, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform Platform Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants