mock-server/mockserver-node

Breaking API change for java system properties made in 5.4.1

hennr opened this issue · 6 comments

hennr commented

With the 5.4.1 release you changed the name for the passed java system properties to mockerserver-*.jar from systemProperties to jvmOptions:

e2c0a86#diff-168726dbe96b3ce427e7fedce31bb0bcL166

This silently changes and breaks the "API" which led to some time consuming investigations on my side.

It even breaks the behaviour found in mockserver-client-node which depends on the old syntax:
https://github.com/jamesdbloom/mockserver-client-node/blob/11441202cd998a8f3486308973872491dcedc86c/Gruntfile.js#L33

A good fix IMO would be to stick with one solution and add at least a logging message for everyone who uses the wrong option, be it systemProperties or jvmOptions. I can even think of failing instantly if the wrong option is set as this is really bad to find after an update.

To avoid these errors in future releases a function to pass java system properties down to mockserver-*.jar would be very nice.

Cheers

Perhaps you could provide a PR to test if the wrong property is provided and output an error message informing users?

And another PR that fixes the inconsistency with mockserver-client-node?

hennr commented

Yes, I could give it a try on monday.
I assume that you want to go with jvmOptions instead of systemProperties then?

P.S. I already made an unrelated PR for mockserver-client-node in the meantime.

Thanks I have merged all your PRs, and yes I think jvmOptions is clearer for a none JVM audience, because systemProperties is very JVM specific.

I may be able to find time add a check for use of the previous value to alert people to the breaking change, or a PR would be welcomed.

hennr commented

Yep, thanks for merging my PRs!

I PR for the this issue is already in the making but #12 got into my way while writing a test.
So if you could fix #12 I will send a PR for #10 soon.

hennr commented

Hi @jamesdbloom , as #12 is fixed by now I opened an PR on this issue.

hennr commented

@jamesdbloom any chance to see this merged within the next days?
Thanks in advance!