-
Notifications
You must be signed in to change notification settings - Fork 59
implement version override per SV #3507
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
base: main
Are you sure you want to change the base?
Conversation
[static] Signed-off-by: Mateusz Błażejewski <mateusz.blazejewski@digitalasset.com>
|
I'm not sure if I should account here for resources from |
Argh sorry for taking so long to get to this...
Irrelevant for SVs
Super relevant for SVs... so we definitely need the version of that (for a given SV) to match the version we're deploying (for that SV) from |
martinflorian-da
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very reasonable so far, thank you! Not entirely opposed to merging this without sv-canton for now, for smaller PRs. Up to you... (not approving in case you decide to make a bigger PR).
In any case: I'd strongly suggest doing a hdm test before merging anything related to versions...
Also some thoughts on sv-canton, for when you get to it:
- you probably want to ignore the override when deploying legacy or upgrade nodes, and apply it only on active; not sure if this will ever matter in practice, but semantics-wise it seems most reasonable that way?
- it might be a candidate for a unit test to make sure the override gets applied to all deployed releases where it matters (participant and
splice-global-domain...); you'll probably be in a better position to judge whether it's worth it
Consider confirming that this works as expected at least once manually before claiming that. |
[static] Signed-off-by: Mateusz Błażejewski <mateusz.blazejewski@digitalasset.com>
[static] Signed-off-by: Mateusz Błażejewski <mateusz.blazejewski@digitalasset.com>
|
A test here seems like a good idea. To do that I'd need #3507 merged first though. |
|
/hdm_test |
|
Deploy HDM pipeline triggered for Commit ce466931786248a94fd80c5283e894ffe548e752 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/47546 |
|
/hdm_test |
|
Deploy HDM pipeline triggered for Commit ce466931786248a94fd80c5283e894ffe548e752 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/47722 |
fixes #3488