-
Notifications
You must be signed in to change notification settings - Fork 519
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
Parameterize admin and control container URIs on region #638
Conversation
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.
🌊
92b66d3
to
929b5eb
Compare
This push rebases onto #637 to get the typo fix caught by zmrow. |
workspaces/models/defaults.toml
Outdated
[metadata.settings.host-containers.admin.source] | ||
setting-generator = "parameterize settings.host-containers.admin.source" | ||
template = "328549459982.dkr.ecr.REGION.amazonaws.com/thar-admin:v0.2" | ||
parameters = '{"REGION": "settings.aws.region"}' |
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.
My only nit here is the use of settings.aws.region
in a global defaults.toml
but we're planning to refactor that to support variants, right?
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.
At the moment, every variant is aws, so this default is in the common location; we'd move it out to aws-specific variants as soon as we have something else. It might help if we have an "include" feature, or something similar, but we don't have to decide on that quite yet.
d5aa94d
to
6d03f28
Compare
929b5eb
to
d9b7706
Compare
This push just rebases on the changes from #637. |
d9b7706
to
67a403b
Compare
This push rebases onto the new template syntax from #637. I apologize for the rebase being squashed with the changes; the only changes were in defaults.toml, where you now see just a "template" key and no longer the "parameters" key. |
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.
🌡
67a403b
to
a67331d
Compare
This push just rebases on a typo fix from #636. |
a67331d
to
e20a6b4
Compare
This push is just a rebase onto changes from #637. |
e20a6b4
to
f0133b3
Compare
Just a rebase on develop because of #637, no changes. |
Builds on #636 and #637.
Fixes #231.
Testing done:
See #637. This is a separate PR to facilitate any potential discussion around actually enabling the feature, versus adding the capability, but this had to be there for testing #637.