-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat(registry): add object storage templates #123
feat(registry): add object storage templates #123
Conversation
70b1560
to
8dd5ea4
Compare
This solves deis#95 for registry. And configuration is generic for other object storage backends, as well.
8dd5ea4
to
04d1e6e
Compare
should we also remove https://github.com/deis/charts/blob/master/deis-dev/manifests/deis-registry-rc.yaml since that'll get overwritten? |
@bacongobbler Good idea. Done. |
secret should have the namespace "deis" |
Thanks, @kmala. Fixed that. |
In the toml we need to provide the json between ''' ''' instead of " ".On making this change and testing, everything is working fine. |
This is a nice abstraction point. Is this how we're planning on creating and sharing S3 credentials across components? If so there's a few things I'll have to change in deis/postgres to make it aware of this secret, as well as other components so the user has only one place they need to dump their S3 credentials. If it's not a beta requirement we could just let users set up each component separately (as much as that's a hassle) then clean it up before stable. |
labels: | ||
heritage: deis | ||
annotations: | ||
deis.com/objectstorage: "{{.storage}}" |
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.
Elsewhere, we have used deis.io (or subdomains thereof) as a prefix. I vaguely recall some short discussion about this a couple months back where we had agreed .io was better in this context because it was more open-sourcey. That aside, now that .io's been used elsewhere, it's more a matter of consistency 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.
Okay. Will fix that
3e2de85
to
9ad6afa
Compare
@kmala has manually tested, so I'm removing that flag. All other issues have been fixed as well. |
Love this change. I always loved stateless platform and this gets us a step closer to the possibility of doing that. 💯 |
…zation feat(registry): add object storage templates
If I want to use filesystem (default) storage now, how would I do that? @krancour, @kmala, @technosophos: how did you test that configuration? Looks broken to me: $ helm generate deis-dev
[ERROR] Error opening value file: Near line 45 (last key parsed 'gcs.key_json'): Expected value but found '`' instead.
[ERROR] Failed to complete generation: failed to execute helm template -o /Users/matt/.helm/workspace/charts/deis-dev/manifests/deis-registry-rc.yaml -d /Users/matt/.helm/workspace/charts/deis-dev/tpl/objectstorage.toml /Users/matt/.helm/workspace/charts/deis-dev/tpl/deis-registry-rc.yaml (/Users/matt/.helm/workspace/charts/deis-dev/tpl/deis-registry-rc.yaml): exit status 1 |
@mboersma I tested this with the default (file system) and it worked for me. I'd be happy to try again to see if I can repro your issue. |
I just realized I tested this in the time between some comments I'd made and subsequent force push that introduced the issue. That explains it. |
This configuration is for registry. However, configuration is generic for other object storage users, as well.
Closes #95
TO TEST:
helm up
your Deis charts repohelm fetch deis/deis-dev
cd $(helm_home/workspace/charts/deis-dev)
tpl/objectstorage.toml
. Directions are in the file.helm generate -x manifests deis-dev
Then you should have installable manifests.