ADRs: Filling in impact sections for 001 - 006 #1416
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tjohnson7021
changed the title
tjohnson7021
requested review from
basiliskus,
sfradkin,
luis-pabon-tf,
halprin,
saquino0827,
somesylvie,
jbiskie,
brick-green,
pluckyswan,
GilmoreA6,
jherrflexion and
scleary1cs
somesylvie
reviewed
Oct 14, 2024
adr/002-java.md
Outdated
Comment on lines
31
to
32
| - Code tends to be more verbose leading to longer development times and more boilerplate code. | ||
| - Code tends to be more verbose which can lead to longer development times and more boilerplate code. |
There was a problem hiding this comment.
These two statements are basically the same
Is it worth adding an additional negative, that the learning curve may be steep for new engineers on the team who are less familiar with Java?
There was a problem hiding this comment.
That was probably a side effect of my drafting - Thanks!
I definitely agree on adding the second comment. I do remember reading that.
adr/005-oesa.md
Outdated
| @@ -22,7 +22,27 @@ There are a couple main concepts that we strive to maintain while also being pra | |||
| less important code depending on more important code. For example, when business logic needs to call the | |||
| database. | |||
|
|
|||
| ### Related Issues | |||
| ## Impact | |||
There was a problem hiding this comment.
This comment might be out of scope for this particular PR, but when I first started, I couldn't even figure out what OeSA was. It would be really helpful to add some more context on it (at the very least, that it means 'option-enabling software architecture', plus probably that it's a Flexion thing, and since the Flexion info is all non-public, maybe like a two sentence summary of what it is
There was a problem hiding this comment.
I don't think it's specifically a Flexion-thing per say? I know it employs a lot of the Clean Coding practices from the famous Uncle Bob. I need to verify if OeSA is even still the name, but when/if correct - will add that. I think the concept may have evolved a bit since then as the content is being honed.
@halprin @sfradkin , maybe you can correct me if I'm completely out of order?
There was a problem hiding this comment.
It would be really helpful to add some more context on it (at the very least, that it means 'option-enabling software architecture'...
Also forgot - the title shows the full acronym already. Is that okay?
There was a problem hiding this comment.
I made a chunk of edits to the OEA adr. Open to suggestions after a re-review.
adr/004-groovy-spock.md
Outdated
| ### Risks | ||
|
|
||
| - Tests can run slow | ||
| - Dependency management difficulties |
There was a problem hiding this comment.
Can you expand on this risk?
adr/006-domain-plugin.md
Outdated
Comment on lines
27
to
30
| Encapsulation of domain-specific logic: Ensuring isolation of concerns; Core application logic remains unaffected | ||
| Reusability: Plugins can be reused reducing duplication | ||
| Flexibility: Domains can evolve independently and potentially be extracted into separate projects. | ||
| Targeted testing: Focused unit and integration testing |
There was a problem hiding this comment.
These should either be bullet points or get an extra line break between them. Right now they're all running together
adr/006-domain-plugin.md
Outdated
| ### Negative | ||
|
|
||
| - Increased complexity: Managing multiple plugins can increase architectural complexity over time, especially with the addition of features. | ||
| - Dependency management challenges: Ensuring compatibility across multiple version can become cumbersome |
pluckyswan
reviewed
Oct 14, 2024
There was a problem hiding this comment.
In the Context section, this sentence confuses me: The trusted intermediary useful for just one business domain. There are multiple areas in healthcare where a trusted intermediary could be beneficial.
There was a problem hiding this comment.
Thank you! I didn't touch those sections at all so thats another thing I'll add to the ADR formatting checklist I made.
sfradkin
reviewed
Oct 15, 2024
- changed to new name 'OEA' (Option Enabling Architecture) - fixed formatting
- Cleaned up original record - Added helpful guidelines for ADR generation
- updated files to be more uniform
* Added new module rs-e2e for RS Integration Tests in Staging * Added initial implementation to pull files from Azure storage container * Added local file fecther, hl7 file matcher, and first test assertion on matched files Co-authored-by: Sylvie <sschuresko@flexion.us> * Cleaned up and simplified logic * Clean up: remove comment * start copying rule engine stuff from etor project Co-Authored-By: Basilio Bogado <541149+basiliskus@users.noreply.github.com> * first draft of assertions Co-Authored-By: Basilio Bogado <541149+basiliskus@users.noreply.github.com> * Renamed 'transformation' to 'assertion', plus some cleanup * Some more refatoring to adapt the transformation engine to be used for HL7 assertions * Turned arrays in definitions into key value pairs * Updated the assertion definitions to use new syntax and added initial implementation for the hl7 expression evaluator Co-authored-by: Sylvie <sschuresko@flexion.us> * Further implementation for the hl7 expression validator. Got a test expression working for equal comparison between hl7 field and a literal value Co-authored-by: Sylvie <sschuresko@flexion.us> * Cleanup * Some refactoring and cleanup for parseAndEvaluate * Added implementation for evaluateMembership and removed Optional return value Co-authored-by: Sylvie <sschuresko@flexion.us> * Added method to evaluate count operation + cleanup * Use both input and output messages, add a few more statements to test Co-Authored-By: Basilio Bogado <541149+basiliskus@users.noreply.github.com> * Start making AssertionRuleEngine work and some associated cleanup Co-Authored-By: Basilio Bogado <541149+basiliskus@users.noreply.github.com> * Start using the rules file, doing a little cleanup and troubleshooting Co-Authored-By: Basilio Bogado <541149+basiliskus@users.noreply.github.com> * Fixed loading of the definitions file and dependency injection * Fixed null pointer exception. All test assertions are now passing * Added missing assertion * Added missing catch of NumberFormatException * Trying to fix issue with sonar * Added HL7FileStream record to be able to later log the file name Co-authored-by: Sylvie <sschuresko@flexion.us> * Refactored, added logging and fixed dependency injection Co-authored-by: Sylvie <sschuresko@flexion.us> * Added unit tests for the asserion rules engine Co-authored-by: Sylvie <sschuresko@flexion.us> * Catched missing exceptions Co-authored-by: Sylvie <sschuresko@flexion.us> * Refactored to use singleton pattern and dependency injection system for consistency Co-authored-by: Sylvie <sschuresko@flexion.us> * Refactored to reduce code duplication and merge rule engine framework Co-authored-by: Sylvie <sschuresko@flexion.us> * Further refactoring to abstract and reduce code duplication among rule engine frameworks. Still pending to integrate the transformation and validation rule engines to use the new classes Co-authored-by: Sylvie <sschuresko@flexion.us> * Further refactoring to integrate the transformation and validation rule engines to use the new classes Co-authored-by: Sylvie <sschuresko@flexion.us> * Fixed tests in rs-e2e + cleanup * Refactoring to improve null handling * Added test cobverafe for HapiHL7ExpressionEvaluator and HapiHL7FileMatcher Co-authored-by: Sylvie <sschuresko@flexion.us> * Cleanup and move class to appropiate location * Added some of the remaining test coverage * Added test coverage * Updated assertion definitions Co-authored-by: Sylvie <sschuresko@flexion.us> * Added sample file to test UCSD transformations Co-authored-by: Sylvie <sschuresko@flexion.us> * Updated message to avoid sending to ucsd Co-authored-by: Sylvie <sschuresko@flexion.us> * Fixed arguments order Co-authored-by: Sylvie <sschuresko@flexion.us> * Implemented a simple HL7 parser given we can't get what we need from the Hapi library * Added missing error handling * Removed unused code and fixed failing tests Co-authored-by: Sylvie <sschuresko@flexion.us> * Added unit tests in rs-e2e to the allUnitTests task Co-authored-by: Sylvie <sschuresko@flexion.us> * Fixed regex Co-authored-by: Sylvie <sschuresko@flexion.us> * Added github workflow to run the automated test Co-authored-by: Sylvie <sschuresko@flexion.us> * CHanged one assertion to fail to test the workflow Co-authored-by: Sylvie <sschuresko@flexion.us> * Tests are still passing in the PR. Trying something else Co-authored-by: Sylvie <sschuresko@flexion.us> * Updated MSH-10 ids for sample messages + cleanup Co-authored-by: Sylvie <sschuresko@flexion.us> * Need more output from log to get information about the reason for test failure * Way too much output in the logs with --info * Reverted the change to make test fail and updated cron job to run 2 hours after the staging flow run * Removed pull_request trigger used for testing * Add additional test coverage * Added missing javadocs + cleanup * Fixed merge conflicts * Renamed 'result' to 'ORU' to avoid confusion * Moved HapiHL7FileMatcher and HL7FileStream to rs-e2e project given they are not used outside of this project * Updated to follow convention * Explicitly use UTF-8 charset * Move injections before public methods * Updated language to capture what is really the issue here * Simplified logic * Set all patterns as private static final * Missed one Pattern * Changed ArrayList for Set, which is more appropiate in this case * Moved HapiHL7ExpressionEvaluator and HapiHL7Message to rs-e2e project * Added back JavaDocs * Avoid code duplication * Need to keep inputstream open until used * Added readme files for the rs-e2e project and the sample files Co-authored-by: Sylvie <sschuresko@flexion.us> * Added ADRs for the assertion engine and the automated rs integration test. Some more work to be done on the integration test one Co-authored-by: Sylvie <sschuresko@flexion.us> * Added integration test section in the main readme Co-authored-by: Sylvie <sschuresko@flexion.us> * Fixed merge conflicts * Added a couple more assertions for UCSD transformations * Added more to the integration test ADR. Still some more pending Co-authored-by: Sylvie <sschuresko@flexion.us> * Update shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/wrappers/HealthDataTest.groovy Co-authored-by: Jorge Lopez <49923512+jorg3lopez@users.noreply.github.com> * More updates to ADR * Renamed readme.md => README.MD for consistency and changed urls to use relative path from the root of the repo * Missed on last commit * Formatting * Added a few more impact points * Capitalized file extenesion by mistake * Trying to update file extension capitalization * Trying to get git to catch the file extension capitalization * Reverting workaround to get git to catch the file extension capitalization --------- Co-authored-by: Sylvie <sschuresko@flexion.us> Co-authored-by: Jorge Lopez <49923512+jorg3lopez@users.noreply.github.com>
…8.0 (#1385) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Jorge Lopez <49923512+jorg3lopez@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Updated test files msh-11 to be either T or N * Updated test files msh-11 to be D instead of T * Updated MSH-11 from D to T for CA sample OML * Removed hurl script MSH overwrite that is not needed now that we're using MSH-11 for test message routing * Added docs in the readme * Updated readme and added ADR skeleton * Updated MSH-11 to D for files in examples folder * Added ADR * Added to the readme * Moved readme section up to make it more visible * Trying to fix formatting * Minor phrasing updates * Added some more context to ADR * Fixed path and added and added link to ADR * Added additional context in the ADR on the differences between prod and non-prod environments * Fixed path
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
- changed to new name 'OEA' (Option Enabling Architecture) - fixed formatting
- Cleaned up original record - Added helpful guidelines for ADR generation
- updated files to be more uniform
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Samuel Aquino <saquino@flexion.us> Co-authored-by: jherrflexion <118225331+jherrflexion@users.noreply.github.com> Co-authored-by: Bella L. Quintero <96704946+pluckyswan@users.noreply.github.com>
--------- Co-authored-by: Bella Luz Quintero <bquintero@flexion.us> Co-authored-by: saquino0827 <saquino@flexion.us> Co-authored-by: James Herr <jherr@flexion.us> Co-authored-by: halprin <halprin@users.noreply.github.com>
…DCgov/trusted-intermediary into devex/1247-add_impact_sections
|
30 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Filling in Impacts of ADRs that are missing an 'Impact' section
NOTE: When you review, please keep in mind, I am using a mix of general research and experience to fill in these sections, so they may be lacking context-specific impacts of which I am unaware.
Issue #1247
Checklist