[tracer] Add support for RID v22a #123
Merged
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
mickmis
reviewed
Jul 7, 2023
There was a problem hiding this comment.
Looks good in general. I have two main comments:
- (c.f. a comment in the review) the implementation of
tracer_rid_isa_notificationshould be using IMO the code frommonitorlibinmutate.rid.ISAChangeNotification - is there an easy way to not have separate containers for v19 and v22a, i.e. have all the endpoints for both versions in a single instance? Given that there are also the SCD endpoints in there which don't depend on the RID version I find that slightly confusing. This would also reduce the overall complexity of the deployment.
Co-authored-by: Mickaël Misbach <[email protected]>
mickmis
approved these changes
Jul 7, 2023
There was a problem hiding this comment.
LGTM after clarifications.
Note: the CI fail because of the addition of --chown, which actually requires buildkit to be used. Using buildkit in the CI would definitely be a good idea, but that's not the scope of this PR, I'd suggest maybe then leaving a TODO in the Dockerfile.
As agreed, will be addressed by #124
I don't see any use case were v19 and v22a should run together. The problem is that the tracer is subscribing to the DSS and it has to know which standard (and endpoint) to use. I privileged to keep the implementation simple knowing that v19 will probably not be used for a long time. |
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.
This PR adapts the tracer to support RID v22a. Compatibility with v19 should have been preserved.
Note that it fixes the permissions of
/app/health_check.shfile in the docker image to allow the mock_uss start.sh script to overwrite it properly.Switch of the CI pipeline to build v22a will performed by #119 after merging this PR.