V5.y.z/house keeping#55
Open
gimlichael wants to merge 6 commits into
Open
Conversation
Consolidate reflection utilities into upstream Cuemon.Reflection package. The AssemblyContext class and its comprehensive test suite are removed from Savvyio.Core; callers must migrate to Cuemon.Reflection.AssemblyContext and update CurrentDomainAssemblies (property) to GetCurrentDomainAssemblies() (method).
Replace internal Savvyio.Reflection.AssemblyContext with external Cuemon.Reflection.AssemblyContext across all call sites. Updates SavvyioOptionsExtensions and both MessageConverter implementations (Newtonsoft.Json and Text.Json) to call GetCurrentDomainAssemblies() method instead of the removed CurrentDomainAssemblies property.
Add optional run_mac_tests workflow dispatch input (default false) to control macOS test matrix execution and reduce CI costs. Introduce new test_qualitygate job that centralizes test result evaluation using require_success and require_success_or_skip helper functions. Refactor sonarcloud, codecov, codeql, and deploy jobs to depend on test_qualitygate instead of enumerating individual test jobs, simplifying workflow dependencies and improving maintainability.
Update LocalStack container image from 4.14.0 to 2026.05.0 in both Dockerfile.localstack and docker-compose.yml for access to latest AWS service emulation features and security updates.
Document breaking AssemblyContext removal, refactoring of call sites to use Cuemon.Reflection, CI pipeline improvements with optional macOS testing and centralized quality gate job, and LocalStack Docker image version bump. Add migration guidance for consumers affected by the breaking change.
Greptile SummaryThis PR consolidates the internal
Confidence Score: 4/5Safe to merge once the integration test suite confirms the LocalStack 2026.05.0 image behaves correctly with the existing SNS/SQS tests. The code changes are a clean mechanical migration — the old Savvyio.Reflection.AssemblyContext is deleted and every call site moves to the upstream API with no logic changes. The CI restructuring is well-reasoned: test_qualitygate correctly separates unconditional jobs from opt-in ones, and all downstream jobs now depend on it cleanly. The only area that warrants a second look is the LocalStack image jump from 4.14.0 to 2026.05.0; if the integration tests pass against the new image, the change is fully validated. .github/workflows/ci-pipeline.yml (quality gate logic) and Dockerfile.localstack / docker-compose.yml (LocalStack version bump). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
init[init\ncalculate variables] --> build
init --> test_mac_cond{run-mac-tests\n== true?}
build --> test_linux
build --> test_windows
build --> test_mac_cond
build --> integration_test_cond{run-privileged-jobs\n== true?}
build --> integration_test_rabbitmq
build --> integration_test_nats
prepare_test --> test_linux
prepare_test --> test_windows
test_mac_cond -->|yes| test_mac
test_mac_cond -->|no, skipped| QG
integration_test_cond -->|yes| integration_test
integration_test_cond -->|no, skipped| QG
test_linux --> QG
test_windows --> QG
test_mac --> QG
integration_test --> QG
integration_test_rabbitmq --> QG
integration_test_nats --> QG
init --> QG
QG["test_qualitygate\nif: always()\nrequire_success: linux, windows, rabbitmq, nats\nrequire_success_or_skip: mac, integration"]
QG -->|success| sonarcloud
QG -->|success| codecov
QG -->|success| codeql
QG -->|failure| BLOCKED([pipeline blocked])
sonarcloud --> deploy
codecov --> deploy
codeql --> deploy
build --> pack --> deploy
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
.github/workflows/ci-pipeline.yml:319-320
**RabbitMQ/NATS jobs are always required but have no skip path**
`integration_test_rabbitmq` and `integration_test_nats` have no `if:` guard, so they always run on every trigger (including fork pull requests). `require_success` is therefore the right gate for them. However, if either job is ever given a conditional guard in the future, this call site would need to be upgraded to `require_success_or_skip` — unlike `integration_test`, there is no `_ENABLED` flag wired here. Worth keeping this asymmetry in mind if the RabbitMQ or NATS jobs are later made optional.
### Issue 2 of 2
Dockerfile.localstack:2
**Large LocalStack version jump may affect AWS service emulation**
The image moves from `4.14.0` to `2026.05.0`, which appears to be LocalStack's calendar versioning scheme and represents a very large upstream delta. The integration tests exercise `sns` and `sqs`; any breaking changes in request/response shapes, endpoint behaviour, or error codes would only surface at test time. If the integration test suite passes cleanly with this image, the bump is safe — just worth being aware of the scope of the change.
Reviews (1): Last reviewed commit: "♻️ update sonarcloud and codecov conditi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+319
to
+320
| require_success "integration_test_rabbitmq" "$INTEGRATION_TEST_RABBITMQ_RESULT" | ||
| require_success "integration_test_nats" "$INTEGRATION_TEST_NATS_RESULT" |
There was a problem hiding this comment.
integration_test_rabbitmq and integration_test_nats have no if: guard, so they always run on every trigger (including fork pull requests). require_success is therefore the right gate for them. However, if either job is ever given a conditional guard in the future, this call site would need to be upgraded to require_success_or_skip — unlike integration_test, there is no _ENABLED flag wired here. Worth keeping this asymmetry in mind if the RabbitMQ or NATS jobs are later made optional.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci-pipeline.yml
Line: 319-320
Comment:
**RabbitMQ/NATS jobs are always required but have no skip path**
`integration_test_rabbitmq` and `integration_test_nats` have no `if:` guard, so they always run on every trigger (including fork pull requests). `require_success` is therefore the right gate for them. However, if either job is ever given a conditional guard in the future, this call site would need to be upgraded to `require_success_or_skip` — unlike `integration_test`, there is no `_ENABLED` flag wired here. Worth keeping this asymmetry in mind if the RabbitMQ or NATS jobs are later made optional.
How can I resolve this? If you propose a fix, please make it concise.| @@ -1,5 +1,5 @@ | |||
| # Use the LocalStack base image | |||
| FROM localstack/localstack:4.14.0 | |||
| FROM localstack/localstack:2026.05.0 | |||
There was a problem hiding this comment.
The image moves from 4.14.0 to 2026.05.0, which appears to be LocalStack's calendar versioning scheme and represents a very large upstream delta. The integration tests exercise sns and sqs; any breaking changes in request/response shapes, endpoint behaviour, or error codes would only surface at test time. If the integration test suite passes cleanly with this image, the bump is safe — just worth being aware of the scope of the change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Dockerfile.localstack
Line: 2
Comment:
**Large LocalStack version jump may affect AWS service emulation**
The image moves from `4.14.0` to `2026.05.0`, which appears to be LocalStack's calendar versioning scheme and represents a very large upstream delta. The integration tests exercise `sns` and `sqs`; any breaking changes in request/response shapes, endpoint behaviour, or error codes would only surface at test time. If the integration test suite passes cleanly with this image, the bump is safe — just worth being aware of the scope of the change.
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
- Coverage 98.40% 98.38% -0.03%
==========================================
Files 177 176 -1
Lines 3773 3726 -47
Branches 363 355 -8
==========================================
- Hits 3713 3666 -47
Misses 58 58
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request focuses on consolidating the internal
AssemblyContextreflection utility into the upstreamCuemon.Reflectionpackage and tightening the CI pipeline structure. It also updates dependencies and improves the workflow for running tests, especially for macOS. The most important changes are summarized below.Reflection Utility Consolidation
AssemblyContextclass fromSavvyio.Core(Savvyio.Reflectionnamespace) and updated all usages to referenceCuemon.Reflection.AssemblyContext.GetCurrentDomainAssemblies()instead of the removedCurrentDomainAssembliesproperty. This affectsSavvyioOptionsExtensionsand bothMessageConverterclasses in the Newtonsoft.Json and Text.Json extensions. [1] [2] [3] [4] [5] [6] [7] [8] [9]CI Pipeline Improvements
run_mac_testsboolean input to the workflow dispatch event, allowing macOS tests to be run on demand rather than by default, and updated the initialization logic to set therun-mac-testsoutput accordingly. [1] [2]test_macjob to run only ifrun-mac-testsistrueand made it depend explicitly on theinitjob.test_qualitygatejob that centralizes the evaluation of all test results (Linux, Windows, macOS, integration, RabbitMQ, NATS) using helper functions to require success or allow skips for disabled jobs.sonarcloud,codecov,codeql,deploy) to depend on the newtest_qualitygatejob instead of referencing every individual test job, ensuring a cleaner and more maintainable workflow.Dependency Updates
4.14.0to2026.05.0in bothDockerfile.localstackanddocker-compose.yml. [1] [2]Documentation
CHANGELOG.mddetailing the removal of the internalAssemblyContext, the migration instructions, CI pipeline changes, and dependency updates.