search

faradayio / cage

Develop and deploy complex Docker applications
http://cage.faraday.io
Apache License 2.0
307 stars 26 forks source link

Support service build URLs with subdirectories #82

Closed camjackson closed 6 years ago

camjackson commented 6 years ago

This is a functioning implementation of #81. This isn't quite ready, but it's at a point where I'd like to get some feedback to see if my approach makes sense.

Firstly, this only works for service sources, not lib sources. Lib sources are a bit trickier, seeing as the build field of the lib is separated from the service's configuration. To keep the scope of this down, I was thinking of keeping this PR just for services, and lib subdirectories could be a future thing?

The main thing I'm not happy with is the new example project I added. I put it there purely to have something to run unit tests against. It's working well for that purpose, but it's kind of an un-runnable example, seeing as rails_hello doesn't actually have subdirectories of myfolder and otherfolder. Not sure what the right way forward is on this.

Oh and the other thing is that it depends on the unmerged changes in compose_yml. So this one commit isn't mergeable. I'll have to fix it up later so there's not a failing commit in the git history.

emk commented 6 years ago

Firstly, this only works for service sources, not lib sources. Lib sources are a bit trickier, seeing as the build field of the lib is separated from the service's configuration. To keep the scope of this down, I was thinking of keeping this PR just for services, and lib subdirectories could be a future thing?

Honestly, I'm not really happy about lib sources existing in the first place. They have good reasons for existing, but they feel like a hack. Let's just return a nice error if somebody tries to do this.

The main thing I'm not happy with is the new example project I added. I put it there purely to have something to run unit tests against.

Hmm, good point. Maybe we should have a text/fixtures/some-project directory for things which we use in unit tests but which aren't actual working examples.

camjackson commented 6 years ago

Ok I still need to make it fail on lib source subdirectories, and also move the dodgy example out into a fixtures location, but that last commit was pretty hefty on its own. I'm pretty excited about it, I just hope it wasn't too ambitious!

As mentioned above, I went down the path of replacing the tuple with a struct, and discovered that we construct the Sources object with context: Option<(String, Option<String>, dc::Context)>, which we then yield from the iterator as Option<Result<(String, Option<String>, &'a Source)>>. So the iterator was doing a lookup to convert dc::Context to &Source. What I decided to do was move that lookup into the construction of the Source object, rather than look it up while iterating. Then I did a similar change for the libraries, also looking up the lib by its name when constructing the Source object, rather than while iterating.

The end result of this change is pretty cool IMO. It makes the iterator implementation waaay simpler. Now that it just yields data that it already has, rather than doing fail-able lookups, it doesn't need to return a result any more, which simplifies all its callers too. And finally, it doesn't need to store a reference to the sources::Sources map. Hopefully I didn't get too carried away 😆

camjackson commented 6 years ago

Ok that's everything from me for now. Only other thing will be the compose_yml version bump. Plus any other PR feedback of course :)

No rush by the way, I'm conscious that this PR got a bit bigger and more involved than either of us initially thought it would be! You're definitely right that there are some tricky subtleties here.

emk commented 6 years ago

OK, I've released a new compose_yml with your changes, so you can update that dependency.

Just a quick note: We're about to dive into a giant, multi-person refactoring on an internal project at Faraday. So it may take me a couple of days to find the time to read through the final version of this patch with the detail that such a change deserves.

You are absolutely encouraged to ping me on this PR around EOD Friday if you haven't heard anything from me.

camjackson commented 6 years ago

Thanks for that! I already pushed the commit with the version bump :) All commits prior to that one would have been broken without the necessary extra features, so this PR should probably be squashed before merging.

Thanks for letting me know, and good luck with the mega refactoring!

camjackson commented 6 years ago

Ping 😊

camjackson commented 6 years ago

Ready :)

emk commented 6 years ago

This looks great! (And it has been a surprisingly long road for both of us.) I'm going to try to merge this sometime this weekend and get out a new release.

camjackson commented 6 years ago

Gentle reminder of this pull request :)

Let me know if there's anything I can do to make this easier to get through!

emk commented 6 years ago

Oh, no, I thought we had merged and released this! Thank you so much for reminding me. Please feel free to ping me much sooner next time.

I'm debugging load balancers this morning, but I'd like to get this merged either today or tomorrow if possible. Feel free to remind me aggressively if you don't hear from me soon.

(By the way, we're prototyping a couple of major cage enhancements for compiled languages and multi-stage builds internally. I hope to post design RFCs shortly.)

camjackson commented 6 years ago

🎉

Looking forward to the multi-stage stuff too!

emk commented 6 years ago

Ugh, I'm trying to get the build to go green on Travis CI before releasing, and it's basically a recursive "update everything" nightmare.

This make take a while...

camjackson commented 6 years ago

Bummer. Weird CI failures are always annoying to debug.

Can I help at all, or is it just a matter of digging through it step by step?

emk commented 6 years ago

Well, there are several problems, most of them related to building static binaries for Linux and MacOS:

So I'm just taking a deep breath, sitting down, and working through the errors 1-by-1 until I can unwind this stack.

emk commented 6 years ago

OK, it looks like fixing the Mac build to handle HTTPS is going to be fairly ugly, because of https://github.com/faradayio/boondock/issues/14 Let me think about this.

emk commented 6 years ago

I've decided to avoid the boondock issue for now. This means no SSL on the Mac still.

I just put a full day into upgrade most of cage's dependencies, including (most importantly) the serde library. I've also been working on the travis branch to try to get the build green again: https://travis-ci.org/faradayio/cage/builds

Once we get a green tagged build (which means the release binaries worked), then we can release. Unfortunately, I can't afford to put in many more hours today. But if you see something breaking on the travis branch that you think you can fix easily, please feel free to send a PR.

emk commented 6 years ago

OK, I think I've fixed a lot of CI problems, but maybe not everything. The latest attempt is here: https://travis-ci.org/faradayio/cage/builds/317077169

emk commented 6 years ago

OK, I'm in the process of release cage 0.2.4, which includes your changes! The crate should be available within 30 minutes or so, and binaries for Linux and Mac later this evening. (Knock on wood.) If you have any problems, please ping me.

I've updated a huge number of cage's dependencies to the latest versions.

camjackson commented 6 years ago

Wow, that all sounds like a mammoth effort, and probably not a whole lot of fun. Appreciate all the extra work you put into getting this release out. I'm going to test it out right now!

camjackson commented 6 years ago

Works great!

Only thing is that it currently logs this warning on every cage command:

WARNING: docker-compose.yml file validation disabled until valico is updated (from compose_yml::v2::validate)
WARNING: docker-compose.yml file validation disabled until valico is updated (from compose_yml::v2::validate)
WARNING: docker-compose.yml file validation disabled until valico is updated (from compose_yml::v2::validate)
WARNING: docker-compose.yml file validation disabled until valico is updated (from compose_yml::v2::validate)

Looks like you already have an issue open on compose_yml for this: emk/compose_yml#11