Change to use sh instead of bash #206
Conversation
Ensure POSIX compatibility
| # need a Unix path | ||
|
|
||
| if type cygpath > /dev/null 2>&1; then | ||
| if type cygpath >/dev/null 2>&1; then |
There was a problem hiding this comment.
Is this a cosmetic change, or actually required for POSIX compliance?
| PROJECT_GH="databus23/$PROJECT_NAME" | ||
| GREP_COLOR="never" | ||
|
|
||
| : ${HELM_PLUGIN_DIR:="$(helm home --debug=false)/plugins/helm-diff"} |
There was a problem hiding this comment.
We do use HELM_PLUGIN_DIR in installFile. Should we just revert this or just make it POSIX-compliant?
There was a problem hiding this comment.
@mumoshu AFAIK the : denotes the line as pretty much a comment and thus would not run. As helm home is not supported in helm3
There was a problem hiding this comment.
@deiga Thanks. I thought so at first, but my tiny experiment revealed that this seems to have actual side-effect of assigning the value.
$ bash -c ': ${FOO:=somevalue}; echo $FOO'
somevalue
As helm home is not supported in helm3
Good catch! Then it can be said that this can safely be removed for helm 3. Not yet sure that's alright for helm 2 though.
There was a problem hiding this comment.
Right. Should I add a conditional for helm2?
There was a problem hiding this comment.
@deiga That would be awesome! It will allow us to move forward without deciding if it's necessary or not right now.
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
|
@mumoshu Sorry, this took me forever to get back to. It should now work for both Helm v2 and v3 |
|
Any chances this one will be merged? |
mumoshu
left a comment
mumoshu
left a comment
There was a problem hiding this comment.
LGTM. Thanks for your contribution!
3 participants
Ensure POSIX compatibility
Fixes #204