-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Support reading config from stdin #1488
Conversation
651468c
to
e6e1dc0
Compare
|
||
def get_config_path(base_dir, file_path): | ||
if file_path: | ||
return os.path.join(base_dir, file_path) |
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 I'd find this bit of logic less confusing if it were moved into the function above. At that point, I think the functions' responsibilities are a bit better-separated, and they could also be renamed. Say:
def resolve_config_details(base_dir, filename):
if filename == '-':
return ConfigDetails(yaml.safe_load(sys.stdin), os.getcwd(), None)
if filename is None:
file_path = find_config_file(base_dir)
else:
file_path = os.path.join(base_dir, filename)
return ConfigDetails(load_yaml(file_path), os.path.dirname(file_path), file_path)
def find_config_file(base_dir):
(candidates, path) = find_candidates_in_parent_dirs(SUPPORTED_FILENAMES, base_dir)
# ...
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 had just copied the old get_config_path()
from Command
, but now that we have another place to move it, I think that makes sense.
So it won't be possible to use a compose-file and stdin at the same time? This would be interesting when people want to overwrite stuff from compose-file without creating a new file. Such as: $cat my-compose.yml
web:
ports: 1234:5000
$ docker-compose up -f my-compose.yml --stdin-extends '{web: {ports: 5000:5000}}' Which would result in running web with ports 5000:5000 instead of 1234. |
@aanm that's correct, the stdin would replace the file. I think this is pretty standard for unix utilities. If you need something from a file, it would be up to you to read the original file, and merge it into the final config you send to stdin. |
@aanm What you describe is interesting (and partly covered by the @dnephin Come to think of it, what's the behaviour of |
True, I should add that with the small doc change I made about using |
@aanand I thinking that could some some users problems when they want to scale count=0
while true;
let count=count+1
docker-compose up -f my-compose.yml --stdin-extends '{web: {ports: $count:5000}}'
do My point is too use environment variables has input for configuration in compose. I think that would solve some problems that users are getting. @dnephin Does docker itself has this feature that you are PR? |
cb28737
to
4c0736e
Compare
Updated the docs, and cleaned up |
4c0736e
to
37bd76a
Compare
Signed-off-by: Daniel Nephin <dnephin@gmail.com>
37bd76a
to
ae96fc0
Compare
Rebased again! Should be ready |
LGTM |
Support reading config from stdin
Related to #1377
By supporting configuration from stdin, it becomes a lot easier for users with complex templating requirements to use a separate tool for templating, and pass the final config into docker-compose directly.
I wanted to ensure that the
if filename == '-'
check only happened in one place, so I had to refactor some ofcompose.cli.command
.get_config_path
was moved tocompose.config
to support this.