Fix ash template #1303
Fix ash template #1303
Conversation
|
Hi @gregsymons, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
gregsymons
force-pushed
the
fix-ash-template
branch
from
4832578
to
a3b6f1f
Compare
|
Clearly, I didn't understand the tests enough to see that there was already a test. That I have now made fail 😄. I'll be taking a closer look at this closer to the end of the week. |
|
Thanks @gregsymons for starting another take on since 🤗 I guess #1276 is the way forward to split Having said that, I have no idea when this will be ready to merge. So I'm happy to merge your solution to makes things work in the mean time. |
|
BTW, is there a way to see the output from the failure in the checks? Or is my best bet to get it to fail locally and inspect it there? |
Signed-off-by: Greg Symons <gsymons@affinipay.com>
Signed-off-by: Greg Symons <gsymons@affinipay.com>
* The original test only coincidentally passed because the broken arg
parsing didn't actually modify the command-line:
* The -D argument _should_ have been consumed during argument parsing,
but wasn't, so remained in residual.
* The test app would not have outputted the -D argument had argument
parsing been working, since it only output the app args, not the
system properties.
* There is now a specific test each for the system properties and the
residual args.
Signed-off-by: Greg Symons <gsymons@affinipay.com>
gregsymons
force-pushed
the
fix-ash-template
branch
from
a3b6f1f
to
c98f0d2
Compare
|
I've got it working now, so once the checks pass, I'm ready to merge unless you'd like me to make any changes. |
|
Looks like the windows tests are failing for the same reason on other PRs, so I don't think I'm going to chase it down here. I'm ready for review. |
There was a problem hiding this comment.
Thanks for adding the tests @gregsymons 👍
The windows test is failing for sometime now and I haven't had the time to pin this down 😞
This fixes #1302, and, in a much simpler way than #1276, #1264. Unfortunately, I don't understand the tests well enough to figure out where to hang a test for the bashisms specifically, nor the
addResidualchange. If you can give me some tips, I'd be glad to write them if you want me to.