Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
add support for `snap set core proxy.{http,https,ftp,all}` #48
Conversation
ogra1
approved these changes
May 29, 2017
looks good, though there is quite a bit more changes than just adding a proxy, mind mentioning that somehow ?
mvo5
changed the title from
add support for `snap set core proxy.{http,https,ftp,all}
to
add support for `snap set core proxy.{http,https,ftp,all}`
May 29, 2017
zyga
reviewed
Jun 8, 2017
Some comments, I'm very -1 on using more shell in the configure hook. It's not safe, slower and hard to audit.
| +# helper that checks if the given tmp_path can be written | ||
| +is_writable() { | ||
| + tmp_path="$1" | ||
| + printf "" 2>/dev/null >> "$tmp_path" |
zyga
Jun 8, 2017
Collaborator
The name doesn't reflect the fact that the resulting file is created, if missing.
| + mv "${tmp_path}" "${path}" | ||
| + fi | ||
| + else | ||
| + if ! grep -q "^${proxy}=${value}" "$path";then |
zyga
Jun 8, 2017
Collaborator
Some inconsistent spacing before then.
I'm not sure if the grep is right in value of various possible (valid) values of the ${value}. Here it will be treated as a regular expression.
| + os.chmod(mocked_binary, 0o755) | ||
| + | ||
| + def mock_snapctl(self, k, v): | ||
| + self.mock_binary("snapctl", """if [ "$1" = "get" ] && [ "$2" = "%s" ]; then echo "%s"; fi""" % (k, v)) |
| + | ||
| + def mock_snapctl(self, k, v): | ||
| + self.mock_binary("snapctl", """if [ "$1" = "get" ] && [ "$2" = "%s" ]; then echo "%s"; fi""" % (k, v)) | ||
| + |
mvo5
referenced this pull request
in snapcore/snapd
Jul 6, 2017
Merged
snapctl: add new `snapctl internal configure-core` #3568
|
This one will probably be superseeded in favour of snapcore/snapd#3594 |
niemeyer
commented
Aug 8, 2017
|
Closing in favor of integration into snapd. |
niemeyer
closed this
Aug 8, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
mvo5 commentedMay 29, 2017
•
Edited 1 time
-
mvo5
May 29, 2017
This branch adds support for
snap set core proxy.{http,https,ftp,all}plus tests and refactor to DRY in the configure hook.