New version of Fetch fails with Gruntwork Installer Fetch version about fetch HOT 11 CLOSED

Perhaps this is some unintended side effect of https://github.com/gruntwork-io/fetch/releases/tag/v0.3.6? Out of curiosity, does it work with v0.3.5?

from fetch.

Confirmed it works on v0.3.5

from fetch.

Thx for checking! Def sounds like some bug was introduced in #55. @vedala Any chance you recognize what the issue is off the top of your head? If not, we'll have to dig in ourselves.

@Merlz Out of curiosity, was there something in 0.3.7 that you needed?

from fetch.

The --source-path flag is required for pulling certain files from our own internal repos.

from fetch.

@brikis98 The error encountered by @Merlz is definitely due to changes I made in #55.

Based on a comment in gruntwork-install code, looks like gruntwork-install expects fetch to accept empty value for --branch option:

https://github.com/gruntwork-io/gruntwork-installer/blob/d7f6de01d7def76cdb21fc508a43b43e5e3511bc/gruntwork-install#L118-L119

I feel the fix I made in PR #55 is not really needed. Working backwards, I feel the bug reported in issue #31, was not really a bug. If this is needed urgently, I can submit a PR that rolls back the changes. But, to be completely sure, I think we should review the fetch's expected and actual behavior and then decide whether issue #31 should be considered a bug or not.

from fetch.

@vedala Thanks for looking into it! Yes, gruntwork-install does expect empty values to work. That way, you can set --branch, --tag, etc to variables that might be empty, and so long as one isn't empty, it should still accept it. Otherwise, we'd have to include lots of conditional logic in the gruntwork-install calls, which would prevent them from being simple one-liners. Perhaps the intended fix for #31 was that if all the values are empty, the error message should disambiguate between the arguments not being set, versus the arguments being set to empty values? @josh-padnick Is that what you had in mind?

from fetch.

@brikis98 It is likely I interpreted the bug incorrectly. Your suggestion sounds right, I will wait for confirmation @josh-padnick and submit a PR with fix.

from fetch.

@josh-padnick Could you chime in here?

from fetch.

You know, when I was looking through #31 myself, I had the same thought as @vedala: this isn't really an error. I'm sorry to create extra work for everyone, but I would be in favor of rolling back #55, especially if it's causing regressions.

from fetch.

@josh-padnick No problem. Thank you for confirming. I will roll back my changes made in #55 and send in a PR soon.

from fetch.

https://github.com/gruntwork-io/fetch/releases/tag/v0.3.9

from fetch.