Strip the `build` field from the generated compose files

[camjackson]

Resolves #69. Obviously not intended to be merged as-is, though I have tested it and it does indeed strip the build field as intended.

Just thought I'd throw something out there. It's a pretty naive implementation, involving some copy-paste from the existing labels plugin.

Is this heading in the right direction, or is it way off? 😄

[emk]

Thank you! This feels like it's moving in the right direction. Some ideas:

• We would want to skip this for cage build.
• We would want to test that this field was present when it was supposed to be, and absent on at least one other invocation.

Do we already have enough testing infrastructure to make this easy to test?

I definitely think this is worth pursuing! Thank you for looking into it.

[camjackson]

Yeah I haven't yet looked at how to conditionally transform based on the current subcommand. Is there anything similar existing in the code already?

Haven't really looked at testing yet. Hoping it's not too hard to add a low-level unit test on the conditional transformation. The trickier one would be some sort of integration test that verifies the end-to-end-behaviour based on whether it's a build command or something else. Not sure if the infrastructure is there already for that sort of high-level integration test.

[emk]

There are no CLI-level tests for cage, though I intend to add some with cli_test_dir the next time I work on it.

Other than that, it should have fairly reasonable test infrastructure IIRC.

[camjackson]

Ok this actually works now!

I still have to write tests for the new code, and fix a few existing tests that I broke.

Question - should the build field be emitted when exporting? Need to figure out the right way to fix the test export_applies_expected_transforms.

Also let me know how you feel about passing the subcommand down through the Project instantiation. I wasn't sure if that's the right approach. Also I was thinking of creating a SubCommand enum, rather than passing strings around everywhere.

[camjackson]

Ok yeah, putting the subcommand on the context made things much cleaner, thanks for that suggestion :)

What are your thoughts on this now? I still need to go through and add the subcommand argument to all of the tests that call Project::output. Also need to add a test for the new plugin itself.

[camjackson]

Ok that's everything from me for now, this is ready for review again.

I took the liberty of adding a Subcommand enum to try to eliminate some of the string matching and comparison that was being done.

[emk]

I need to take a look at this, but I'm crazy busy with another project and can't do it tonight. Please feel free to pester me again next week. :-)

[emk]

Also, we should probably figure out why the Travis build is failing.

[camjackson]

No probs, I don't expect a 24 hour turn-around 😄

Not sure about the travis build. There were a couple of lint errors (unrelated to my changes I think), which I've now fixed. Now it's passing on OSX (which is what I'm developing on), but getting a linker error on linux for this: test src/project.rs - project::Project::from_current_dir (line 160)? That's on stable anyway - beta and nightly have different issues.

Weird 🤔

[emk]

Hmm, that is really weird. I'm seeing similar errors on the PR at #71, now that I look. I may need to dig into this problem myself sometime soon.

[camjackson]

Just rebased this onto master to resolve conflicts from #71. Looks like the build never passed on that PR either.

On a side note - you're not by any chance going to RustFest Zürich next weekend are you?

[emk]

Ugh, sorry about the ongoing build failures. I need to talk to @dkastner about this. Please don't hesitate to ping me next week if you haven't heard from me.

[camjackson]

No worries! Thanks for keeping me updated :)

[camjackson]

@emk Gentle bump on this PR. Would love to see this make into the imminent release if possible.

[emk]

OK, at a very quick glance, I think this patch looks reasonable, but it's pretty big, and it will take me a while to review it. This probably means pushing the release back to tomorrow at the soonest.

[camjackson]

Understood! And again, happy to make changes where needed :)

[emk]

@camjackson No need to rebase this, but could you please merge master into this branch, fix the build errors with run-script, then ping me? We're getting very close.

I'll be debugging TravisCI.

[camjackson]

@emk I fixed that test, build is running now. I'll keep an eye on it.

[camjackson]

The stable build is green! 😄

[emk]

Yay green builds! I'm fixing nightly and trying to figure out why beta keeps failing the same way. We're almost ready.

[emk]

Uh-oh, I'm seeing merge conflicts again, maybe because @dkastner just landed some of his changes. Could you please look? Thank you!

Sorry for this messy merge; we had some communications breakdowns on our end and we're juggling multiple things today. Once we have everything green and merged to master, we'll release.

[camjackson]

No need to apologise! It probably doesn't help that I bundled a bunch of refactoring into this PR. Next time I'll try to keep my PRs as small as possible so they're easier to tackle.

I've just rebased and pushed - the merge conflict was just a silly little thing. Waiting for the build now.

[camjackson]

Beta build still getting stuck waiting for postgres to be ready, even when using the alpine image.

[emk]

Good morning! This looks great, and thank you for the final merge fix.

I had a few minutes to work on this PR, and I ended up making several changes and submitting a new PR as #79. Normally I don't do this, but there was some stuff I wanted to tweak.

• I've removed the later patches with the Subcommand enum. I agree we should use something better than just a String, but I'd rather set up a Subcommand trait with an impl for each subcommand. This could also have a run method that executed the actual command, etc.
• I've put back the unit test for export, and revised it to work with the new code.
• I've squashed the commits (which I don't normally do) because several of the individual commits had broken tests, and I prefer each individual commit has working tests for compatibility with git bisect.

I know that every project likes to handle PRs in its own special, unique way, and that can be a bit of a mess. But just for reference in future PRs, my own personal, idiosyncratic preferences for cage are:

1. Please make sure that each individual commit builds and passes tests, so that git bisect works, and so that I can cherry-pick patches if needed.
2. Once you've submitted a PR and I've reviewed it, please try to avoid rebasing. Thanks to the way the GitHub UI works, it's easy for me to review "new commits" added to a PR, but if the branch is rebased, I need to re-review everything from the beginning.

And as you noted, it usually helps to keep commits small.

Anyway, thank you very much for a great patch! Everybody is going to love this new cage feature, me included. :-)

[camjackson]

Good evening! (It's Thursday evening here in Singapore 🙂)

Understood! Everything you said makes perfect sense. As I get more into OSS I'm getting a much better appreciation for the benefits of a well-maintained git history.

Thanks very much for helping me along with my first contribution. I'm working on my next one already, and I promise it will be a much smaller patch this time! 😄

[emk]

No problems! Everything's just a bit more formal when sending PRs upstream because of the review process, which may mean slightly different git habits than for in-house stuff. This is a great patch, especially for a project in a relatively uncommon language.

(And now that I'm thinking about it, both subcommand: String and the Output mode enum are basically doing different versions of the same thing in the Context, and they should probably both be merged into one Subcommand trait at some point. No rush.)

[camjackson]

Testing something that just worked on another repo that I don't own...

Please unassign me from this issue.

Edit: Ok it didn't work. On another repo I made a similar comment and then it said I unassigned myself, even though I don't have permissions.

Anyway, could you please unassign me? Trying to clean up my issues dashboard on github. Cheers. 🙂