Replace "sed -E" in ghe-host-check with a more portable solution#509
Conversation
| fi | ||
|
|
||
| version=$(echo "$output" | sed -E -n 's/GitHub Enterprise( Server)? version (.*)/\2/p') | ||
| version=$(echo "$output" | head -1 | awk '{print $NF}') |
There was a problem hiding this comment.
This can output various error messages that would get incorrectly matched here. An alternative approach (that avoids using -E in grep too) would be:
| version=$(echo "$output" | head -1 | awk '{print $NF}') | |
| version=$(echo "$output" | grep '^GitHub Enterprise\( Server\)\? version [^ ]*$' | awk '{print $NF}') |
There was a problem hiding this comment.
Good catch there @snh, it didn't turn up in my earlier testing. It looks like the test suite is cranky too. I'll adjust, re-test, and then see if the tests are still failing.
There was a problem hiding this comment.
🤔 I've not tested, but is there a risk this grep is moving the assumption of using a modern variant of sed to grep? I'm thinking some old versions of grep may behave like fgrep so can only use fixed strings.
An alternate solution is the one posed at #480 (comment) (also not tested)
version=$(echo "$output" | sed -n 's/GitHub Enterprise\( Server\)\{0,1\} version \(.*\)/\2/p')
There was a problem hiding this comment.
I tested the alternative solution above on MacOS and it works as expected, but I can't vouch for its portability either—I can probably find out though if we'd prefer to go down that path.
However are we backing ourselves into a corner re: options here by:
- Trying to do the check in one operation; and
- Looking for the word "Server"?
Can we assume that as long as "GitHub Enterprise" is present, it doesn't matter if "Server" is also there?
Why not pull the "GitHub Enterprise" string out first, and then manipulate the result to get the version number. eg:
echo "$output" | grep "GitHub Enterprise" | awk '{print $NF}'
That's a fixed string grep followed by an awk variable that I'm pretty sure has been there for a long time...
There was a problem hiding this comment.
Yeah, I think that would be safe to do.
…mbigious-naming Backport 488 for 3.9: transfer_size are estimated data transfer sizes
3 participants
On older backup hosts,
seddoesn't include the-Eoption which results in aghe-host-checkfailure. While replacingsedwith a newer option would be ideal, it's not always achievable in a short timeframe.The proposed solution requires the version string to be at the end of the first string returned—is that a valid long term assumption? It will cope with any future product name changes more gracefully however, which is what
sed -Ewas added to accomodate.