add wrapper to adjust timedatectl for writable-paths handling (LP: #1650688)#8
Conversation
mvo5
left a comment
mvo5
left a comment
There was a problem hiding this comment.
Approach is fine, but it looks like there are some issues to sort out.
|
|
||
| TIMEDATECTL=/usr/bin/timedatectl.real | ||
|
|
||
| case $1 in |
There was a problem hiding this comment.
Maybe not relevant here because of the inputs we get, but don't we want quotes around $1 and $2 ?
There was a problem hiding this comment.
we want to spellcheck this script please (EDIT: I did mean shellcheck, blame the silly auto-complete)
There was a problem hiding this comment.
Yeah, spellcheck and shell-check
There was a problem hiding this comment.
i merged #16 anything else for spellchecking needed ? (do you want to run spellintian too ?)
note that any spellcheck will choke on the mentioning of "writable-paths" because the original implementation of it is already missing an "e"
|
|
||
| mv /usr/bin/timedatectl /usr/bin/timedatectl.real | ||
|
|
||
| cat >/usr/bin/timedatectl<<EOF |
There was a problem hiding this comment.
Hm, did you test this? A normal "heredoc" will substitute vars, so all the $1, cat >/usr/bin/timedatectl<<'EOF' instead.
There was a problem hiding this comment.
yes, i tested it many times... copied the original timedatectl to $HOME and bind mounted the script on top of /usr/bin/timedatectl after changing the value of TIMEDATECTL to point there.. i didnt test the creation bit though ...
There was a problem hiding this comment.
added the necessary escapes now
|
|
||
| TIMEDATECTL=/usr/bin/timedatectl.real | ||
|
|
||
| case \$1 in |
There was a problem hiding this comment.
Why do you quote everything instead of using:
cat > foo <<'EOF'
No substitution of $1 or $2 or anything with 'EOF'
EOF
? It seems the 'EOF' version is easier to read, no?
There was a problem hiding this comment.
(note the ' around EOF) (as suggested in https://github.com/snapcore/core/pull/8/files#r104106673)
3 participants
No description provided.