Conversation
|
Any thoughts on this? I've been using it on my dev system since june without issue |
|
Hey! Yeah, I just haven't had a chance to take a look yet. I still use mirror daily, but have just been doing the bare minimum for features / project maintenance / etc. I'll take a look at your PR this week. Thanks for the ping! |
stephenh
left a comment
stephenh
left a comment
There was a problem hiding this comment.
Thanks! Really my bad for taking so long to get to this.
Had a few questions about maybe using a proto-based enum to make sending encoding/decoding a little clearer, and potentially reusing / de-duplicating some of tests.
But overall looks great. Let me know what you think re my feedback; if you want to iterate on it, great; if not, I'll probably merge it anyway and maybe hack on the feedback at some point.
| WORKDIR "/opt/mirror" | ||
| COPY --from=mirror-builder /tmp/mirror/mirror ./ | ||
| COPY --from=mirror-builder /tmp/mirror/build/libs/mirror-all.jar ./ | ||
| COPY --from=mirror-builder /tmp/mirror/build/libs/mirror.jar ./ |
There was a problem hiding this comment.
I'm pretty sure these references are fixed in master; can you change back to the mirror-all?
| @Option(name = { "-li", "--use-internal-patterns" }, description = "use hardcoded include/excludes that generally work well for internal repos") | ||
| public boolean useInternalPatterns; | ||
|
|
||
| @Option(name = { "-sd", "--sync-direction"}, description = "direction to sync files, defaults to \"BOTH\", allowed values: \"INBOUND\", \"OUTBOUND\", \"BOTH\"") |
There was a problem hiding this comment.
Hm, so INBOUND is from the perspective of the client, so means "download from server, but don't upload"? Wondering if something like FROM_SERVER or TO_SERVER would be clearer, but that sounds awkward.
There was a problem hiding this comment.
I guess once we get into the MirrorSession side of things, we do need something that is flippable/complement-able, as you're doing.
| @@ -0,0 +1,59 @@ | |||
| package mirror; | |||
|
|
|||
| public enum SyncDirection { | |||
There was a problem hiding this comment.
Protobuf supports enums:
https://developers.google.com/protocol-buffers/docs/proto#enum
If we put this enum in there, then you could just send the sync direction as-is over the wire, and not have to encode it/decode it as the separate allow inbound / allow outbound properties.
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.jooq.lambda.Seq.seq; | ||
|
|
||
| public class UpdateTreeDiffInboundOnlyTest { |
There was a problem hiding this comment.
...that said, its fairly hard to tell what did/did not change between the regular / inbound / and outbound test cases.
I wonder if UpdateTreeDiffInboundOnlyTest could extend UpdateTreeDiffTest and then override the methods that are specifically different?
I've never really done that before (have one unit test extend another and selectively override tests) but I think? it should work.
There was a problem hiding this comment.
I wasn't really sure which properties I was expecting to hold as I did this, so I was using this as a reference more than anything else. That said, you're right about trying to inherit from UpdateTreeDiffTest.
2 participants
still a work in progress, the tests are probably not as complete as they should be, but this seems to be working for me, so I figure it's as good place a place as any to get some feedback.