Skip to content
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

Include external config #758

Closed
wants to merge 4 commits into from
Closed

Conversation

dnephin
Copy link

@dnephin dnephin commented Dec 26, 2014

Resolves #318

Adds support for including other fig.yml files by url (or relative path).

Requires an additional fig.yml key of project-config to support additional configuration. I would see this being used for things like project name as well.

Feedback welcome

@rseymour
Copy link

We already have a solid use case for this functionality (combined with --no-recreate) where we have an etl fig setup that prepares docs to load into an elasticsearch cluster, and an app fig that has an elasticsearch that it is setup to read from. The app fig (which is 4 containers or so) can run independently of the ETL fig setup. Fire up the ETL containers, process some data, shut them down, and the app now has the data brought into it. Very cool imho.

@dnephin dnephin mentioned this pull request Dec 31, 2014
dnephin and others added 3 commits December 31, 2014 15:22
@dnephin dnephin force-pushed the include_remote_config branch from c29e80b to 5b82cdd Compare December 31, 2014 20:23
@dnephin
Copy link
Author

dnephin commented Dec 31, 2014

@aanand
Copy link

aanand commented Jan 2, 2015

OK, just had a read over. Promising stuff. Thoughts, in decreasing order of (aha) import:

  1. This lets you import an entire set of services from another project. Could it be easily extended so that you could import just a defined subset of services? Would people want to do that?
  2. Suppose the file format changes in future such that it contains more than just service definitions. Would users want to be able to import other things from external projects, not just services? Could we make the import mechanism sufficiently generic to support that? (Sorry, I'll try and come up with a good example for this)
  3. Resolution of URLs is really cool, but introduces a bunch of new concerns (most obviously, caching and the management thereof). Could an initial version leave it out and still be useful?
  4. Bikeshedding the project-config key for a bit: could we make it stand out more? I was wondering once about a key for global not-service config being all-uppercase, e.g. PROJECT. (Also, we're using underscores everywhere else, so probably shouldn't introduce dashes here).
  5. I see you're frequently using pop() instead of get() on dictionaries. What's the rationale for mutating loaded data like that? It feels a bit weird to me.
  6. Spotted a couple of typos (pojrectb_webapp, proejctc).

@dnephin
Copy link
Author

dnephin commented Jan 2, 2015

Thanks for the feedback!

  1. I could see the import section also taking a specific list of service names, and the external project only including the named services (and their dependencies). It makes things a bit more complicated, but it might be nice for the case where external configs contain a bunch of extra things. Right now I'm using a small tool which rewrites a fig.yml to replace build directives with image. Since build isn't support from external projects. It might make sense to support this type of filtering in that re-write-fig.yml tool instead (those operations seem related to me). If that's an approach we want to explore, I will work on getting the source up on github.
  2. I suspect importing other settings from the config would not be useful, but it's hard to say until we actually have other things in the config.
  3. Yes, totally agree, the fetching of remote config by url does add a bunch of new concerns. I think for many use-cases it would still be quite useful without this. The fetch of external files could always be done with a curl/s3cmd before running fig. We could exclude it from an initial release.
  4. project-config is easy enough to change. If the config format changes to include a section for services (like the compose proposal suggested), it can be removed entirely, and the top-level key would just be include. It could also change it to PROJECT if that works better. The reason I use a dash is to make sure it's backwards compatible. Someone could potentially have a service with the name PROJECT which would break when they upgrade (however I guess that is pretty unlikely).
  5. Just backwards compatibility (if other code does validation on the expected keys). I think it should be fine to change to a get()



def get_project_from_file(url):
# Handle urls in the form file://./some/relative/path
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle files in local directories? If not, it would be great if it did :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I believe that relative paths were working.

I should say that this implementation will probably change quite a bit. It was more of a proof-of-concept

@sdake
Copy link

sdake commented May 6, 2015

@dnephin reviewed this patch looks solid - although needs a rebase. Can we have this in master soon? :)

@zaneb
Copy link

zaneb commented May 6, 2015

Code looks good to me.

@omerzimp
Copy link

+1 on needing to rebase this.

@scottbelden
Copy link

Any progress on trying to rebase this and get it in? Would love to be able to use this.

@dnephin
Copy link
Author

dnephin commented Jul 3, 2015

@scottbelden Not yet, it still needs a lot of work, and probably the removal of some features for an initial release.

I think I might try and extract this code as a separate tool that can be used to pipe to stdin once #1488 is merged. Once that's ready, I'll leave a comment in this PR.

@dnephin
Copy link
Author

dnephin commented Jul 31, 2015

I've implemented this as an external pre-processing tool at https://github.com/dnephin/compose-addons#dcao-include

If anyone is interested, please check out the docs. Any feedback can be contributed by opening an issue on that repo (and would be appreciated).

@dnephin dnephin closed this Jul 31, 2015
@dnephin dnephin deleted the include_remote_config branch August 24, 2015 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: including external docker-compose.yml
7 participants