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.
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
Run scenarios with CHC enabled #92
base: main
Are you sure you want to change the base?
Run scenarios with CHC enabled #92
Changes from 13 commits
9d851bf
4ab6425
8a58cbb
230fa4f
5a28705
8505e06
3a51e5f
5438ca5
a704319
32f4077
9287afb
ecbb71b
7665694
0e098b4
d4e1cd8
6301aec
b550110
96b0635
e145c02
218d62b
21300d9
bc11dfb
adedffb
7e0f152
6f84ccb
2a25434
c720e1b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What are we going to do about needing different config for some scenarios?
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.
I think the
embed_conductor_config
function might need to be refactored slightly, it should return a type rather than the yaml in the form of a string, so that values can be mutated by scenarios that need custom conductor config different from the one in the yaml files.What do you think?
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.
I'm not sure about the scenario modifying the config. We want to be able to run scenarios with different config. In this case that means we want to run with CHC on and off. We don't then want to be modifying the scenario code to run in different environments etc.
What would be really nice is if you could provide a conductor configuration "overlay". So the
embed_conductor_config
would load a base configuration then at runtime you could configure which overlay to load. It might make sense to merge those as conductor configuration objects rather than yaml.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.
Sorry I am late, got distracted with other work
I see what you mean, what would a good compromise be? perhaps having
-chc
variants of the existing yaml configs, not the cleanest approach.Or rather than reading from the conductor config files, we programmatically build the conductor configs from the scenario, with an env var i.e
CHC_ENABLED
determining whether achc_url
should be set.I am open to discuss this further
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.
When I'm talking about overlays, I mean this kind concept -> https://carvel.dev/ytt/
Not necessarily that tool but something along those lines. So we could have a
with-chc.yaml
and awith-dpki.yaml
that we can choose to load.I think choosing to load that with an environment variable like
CHC_ENABLED
is good. So the current macro can load all the config files and embed them. Then we can choose at runtime which ones we merge with the base conductor config.I know that YAML merging is a bit of a pain in general but if it's purely additive for now, then it shouldn't be so bad I hope.
What do you think?
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.
I see, would the contents of the
with-chc.yaml
andwith-dpki.yaml
only contain the CHC and DPKI configurations?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.
Exactly that, they'd just bring in the piece that they configure. So we might do other things the same way, like different tunings of the network or the conductor itself.
We probably couldn't easily guarantee that a given combination of overlays makes sense but I'm okay with leaving that up to the user for now
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.
Understood. The only question I have now is how to run scenarios with CHC enabled via trycp. I believe the holoports would need to have the reference implementation running?
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.
I think we need to deploy somewhere that the HoloPorts can see yes. We have a couple of servers but nowhere ideal really. I think this might need to be a question we ask to techops
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.