config through env vars #350
Conversation
| @@ -40,7 +45,7 @@ def _replace_env(s): | |||
| if isinstance(s, basestring) and s.startswith('_env:'): | |||
| parts = s.split(':', 2) | |||
| varname = parts[1] | |||
| vardefault = '!ENV_NOT_FOUND' if len(parts) < 3 else parts[2] | |||
| vardefault = None if len(parts) < 3 else parts[2] | |||
There was a problem hiding this comment.
This, and the above changes, are to treat defined but unset config parameters as undefined. This is so that if the config references an environment variable which doesn't exist, we act as if that config param weren't defined.
|
I'd also be up for adding another commit to this PR to update the README. A lot of variables are missing (or are scattered around), and the supported storage engines aren't listed very well. I just want to make sure the code is proper before continuing to documentation. |
|
On Fri, May 09, 2014 at 06:04:59AM -0700, Patrick Hemmer wrote:
Why did you drop the 'exec' in your Dockerfile CMD? I don't see a
I still think it would be better to just have a STORAGE variable STORAGE=glance GLANCE_STORAGE_ALTERNATE=swift
I'd add defaults everywhere we think we have a reasonable suggestion
+1. I expect this will shake up existing configs a bit, so it's |
Just an omission when cleaning it up. I can put it back, or we can go with the
I think I see what you're getting at. The other argument for keeping them separate is to have different defaults for the different storage engines. Like for local storage, |
|
On Fri, May 09, 2014 at 09:23:15AM -0700, Patrick Hemmer wrote:
$ git grep -c elliptics_ so at least for the elliptics backend, you don't have to worry about
I think that small gain is not worth the added layer of config-syntax |
|
@wking I'm concerned about the extent of this PR. I certainly agree both are needed, just probably not in the same timeframe. That being said, this is certainly good work, I just want to make sure that we are not breaking things unexpectedly for people using this, and that breaking changes are introduced timely. What do you think? Also pinging @shin- and @samalba for their opinions Best regards |
|
On Fri, May 09, 2014 at 10:00:35AM -0700, Mangled Deutz wrote:
You can probably do this without much trouble, and you can certainly
But I expect you won't be able to “harmonize the use of env vars” AWS_ACCESS_KEY_ID → S3_ACCESS_KEY should cover almost all users, without needing code in the registry to
I don't see a particular rush in either case, but breaking user-facing |
|
@wking that certainly makes a lot of sense, and we definitely agree here. Now, I think that what we don't want for the future is (I'm sure you will agree as well with that):
Unfortunately, this is (partly) where we are right now. One way to improve might be to ensure that:
Please note I certainly don't want to kill the enthusiasm, or impose my ways on people :-) Best. |
|
On Fri, May 09, 2014 at 10:46:57AM -0700, Mangled Deutz wrote:
That all sounds good to me. I just think it helps to have explicit I'm fine if this PR explicitly punts on environment variable renaming |
|
Ok, i've updated for the Are we settled upon leaving |
|
On Fri, May 09, 2014 at 01:51:18PM -0700, Patrick Hemmer wrote:
I see no reason to add a new config when we can just replace any After a commit like that, I think your current env.yml is mostly a |
|
Ok, I've merged most of into It's a separate commit for now, but if this is the route we go, I'll squash them. |
| @@ -1,20 +0,0 @@ | |||
| test: | |||
There was a problem hiding this comment.
Assuming this is a symlink, seems like it would be cleaner to just remove those files (especially since they were just here for reference in the first place)
There was a problem hiding this comment.
I'm just going off what wking said. If we feel they should be just removed, I can go with that.
There was a problem hiding this comment.
I think it's safe to remove them.
There was a problem hiding this comment.
On Mon, May 12, 2014 at 08:04:15AM -0700, Joffrey F wrote:
@@ -1,20 +0,0 @@
-test:Assuming this is a symlink, seems like it would be cleaner to just
remove those files (especially since they were just here for
reference in the first place)
Maybe you think they were only there for reference, but I don't see
why someone couldn't have been using:
DOCKER_REGISTRY_CONFIG=config_test.yml
or whatever when starting their registry. I don't mind removing the
files, since I still think we're in the “users can be expected to read
release notes” stage of development. However, @dmp42 at least seems
to prefer more backward compatibility 1. Maybe the risk of breakage
for folks using DOCKER_REGISTRY_CONFIG to point at example configs is
acceptable, and we don't need that much backward compatibility?
There was a problem hiding this comment.
Literally the first section of the README advises to do cp config/config_sample.yml config/config.yml, so I think people using DOCKER_REGISTRY_CONFIG to point to a pre-defined sample config will be a very small minority.
|
Ok, LGTM! =) |
|
Just one last thing, now that |
|
I've fixed Anyway, I'll work on updating the README to reflect the changes. |
This adds a default `config.yml` file which supports all options through environment variables.
You can use this to launch a container and set any parameter without having to inject your own config file.
----
This also involved a few code changes.
One is to treat defined but unset config parameters as undefined. This is so that if the config references an environment variable which doesn't exist, we act as if that config param weren't set.
The other is to allow passing the elliptics node list through a string (which is what env vars are). Previously `elliptics_nodes` only accepted a hash/dictionary. This change makes it so that it also accepts a list/array or a string.
For example:
elliptics:
elliptics_nodes:
- 1.1.1.1:2222
- 1.1.1.2:2222
or
elliptics:
elliptics_nodes: "1.1.1.1:2222 1.1.1.2:2222"
config_sample.yml is now the comprehensive config file
|
Ok, I've gone through and updated the documentation. I moved a few things around so that it's not so scattered. I moved all the config stuff that is independent of the storage engine to the top of the 'config options' section. All the storage engines and the parameters they support are then at the bottom. I also added a quick start to the very top. I also added several parameters I ran across during my work that weren't documented. However there are several which I didn't add an explanation on as I'm not sure how to best describe them. These are: the mirroring options, the swift options, the glance options, and cache/cache_lru (whats the difference). Quick link to view README: https://github.com/phemmer/docker-registry/tree/config_cleanup#docker-registry Additional note: I also wanted to remove the numbered lists and use bullet lists instead (but didn't). I personally think they look cleaner, especially once you start nesting. However I didn't want to change formatting since numbered lists were originally used. |
|
On Wed, May 14, 2014 at 04:11:31PM -0700, Patrick Hemmer wrote:
It looks like cache_lru is used to cache files for the storage drivers |
|
Thanks a lot - this is great work. I just have a couple of things blocking this, and would like to merge ASAP:
|
I don't expect this to break anything, and it will get through staging first anyway. |
|
|
|
@phemmer great! |
|
LGTM - let's merge |
This is a PR for #347
This adds a default
env.ymlfile which supports all options through environment variables.You can use this to launch a container and set any parameter without having to inject your own config file.
Example usage:
You can also launch without any settings to get the
dev'flavor' which will launch local storage with sqlalchemy enabled.I tried to get every single config parameter possible, but they are scattered around all over the place, so it is very likely I missed a few.
I also wasn't sure how to go on defaults. This bit should probably be determined and cleaned up. For example in the new
env.yml,s3_encryptdefaults totrueif not specified. I did this as this is the value set inconfig_s3.yml. But I'm not sure if we want to leave this unset and let the code handle it.But then on some things like
config_path, I think a default makes sense. So which parameters get defaults and which don't?The other thing is the env var names. I tried to preserve the existing variable names if they were documented or used already. However the names are rather inconsistent. For example, the s3 bucket var in
config_s3.ymlis$AWS_BUCKET, but in thesetup-configs.shit's$S3_BUCKET. Thenswift_containeris$SWIFT_CONTAINER, but the rest of the vars are$OS_....I personally think it would be best if all the vars for a storage engine were prefixed with that engine name. So
engine: swiftwould be$SWIFT_AUTH_URL, not$OS_AUTH_URL.Lastly I have only tested the
s3,local, anddev'flavors'. I don't have the other services to be able to test them.