Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(16)

Issue 6853075: golxc: added configuration reading (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 7 months ago by TheMue
Modified:
6 years, 5 months ago
Reviewers:
mp+135166
Visibility:
Public.

Description

golxc: added configuration reading Added the reading of configuration files, especially the default configuration at /etc/default/lxc. https://code.launchpad.net/~themue/golxc/002-added-configuration-reading/+merge/135166 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8

Patch Set 2 : golxc: added configuration reading #

Total comments: 1

Patch Set 3 : golxc: added configuration reading #

Patch Set 4 : golxc: added configuration reading #

Total comments: 10

Patch Set 5 : golxc: added configuration reading #

Total comments: 10

Patch Set 6 : golxc: added configuration reading #

Total comments: 2

Patch Set 7 : golxc: added configuration reading #

Total comments: 8

Patch Set 8 : golxc: added configuration reading #

Patch Set 9 : golxc: added configuration reading #

Total comments: 14

Patch Set 10 : golxc: added configuration reading #

Total comments: 4

Patch Set 11 : golxc: added configuration reading #

Total comments: 8

Patch Set 12 : golxc: added configuration reading #

Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -2 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
A config.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +31 lines, -0 lines 0 comments Download
M export_test.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -1 line 0 comments Download
M golxc_test.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +62 lines, -1 line 0 comments Download

Messages

Total messages: 20
TheMue
Please take a look.
6 years, 7 months ago (2012-11-20 14:31:40 UTC) #1
rog
LGTM, but again, i don't know LXC. https://codereview.appspot.com/6853075/diff/1/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/1/config.go#newcode21 config.go:21: // Allways ...
6 years, 7 months ago (2012-11-20 16:20:54 UTC) #2
TheMue
Please take a look. https://codereview.appspot.com/6853075/diff/1/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/1/config.go#newcode21 config.go:21: // Allways check line, could ...
6 years, 7 months ago (2012-11-21 13:02:34 UTC) #3
fwereade
Again, a bit uncertain about the assumptions the tests make about the environment. https://codereview.appspot.com/6853075/diff/3002/golxc_test.go File ...
6 years, 7 months ago (2012-11-21 17:37:11 UTC) #4
TheMue
Please take a look.
6 years, 7 months ago (2012-11-26 10:17:56 UTC) #5
TheMue
Please take a look.
6 years, 7 months ago (2012-11-26 11:04:06 UTC) #6
dfc
Looking good. I think it is more correct to say parsing, rather than reading. https://codereview.appspot.com/6853075/diff/8001/config.go ...
6 years, 7 months ago (2012-11-27 09:18:05 UTC) #7
TheMue
Please take a look. https://codereview.appspot.com/6853075/diff/8001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/8001/config.go#newcode17 config.go:17: env := make(Configuration) On 2012/11/27 ...
6 years, 7 months ago (2012-11-27 09:45:24 UTC) #8
niemeyer
https://codereview.appspot.com/6853075/diff/12001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/12001/config.go#newcode12 config.go:12: type Configuration map[string]string Config for the type, as we ...
6 years, 7 months ago (2012-11-27 17:55:34 UTC) #9
TheMue
Please take a look. https://codereview.appspot.com/6853075/diff/12001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/12001/config.go#newcode12 config.go:12: type Configuration map[string]string On 2012/11/27 ...
6 years, 7 months ago (2012-11-28 10:14:50 UTC) #10
TheMue
Please take a look.
6 years, 7 months ago (2012-11-28 12:38:32 UTC) #11
TheMue
Please take a look.
6 years, 7 months ago (2012-11-28 12:48:07 UTC) #12
rog
https://codereview.appspot.com/6853075/diff/13001/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/13001/environ.go#newcode12 environ.go:12: script := `""set -a; . ` + lxcDefaultFile + ...
6 years, 7 months ago (2012-11-28 12:57:44 UTC) #13
TheMue
Please take a look. https://codereview.appspot.com/6853075/diff/13001/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/13001/environ.go#newcode12 environ.go:12: script := `""set -a; . ...
6 years, 7 months ago (2012-11-28 15:38:45 UTC) #14
fwereade
A few comments: https://codereview.appspot.com/6853075/diff/8003/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode8 environ.go:8: var lxcDefaultFile = "/etc/default/lxc" I'm a ...
6 years, 5 months ago (2013-01-30 16:37:39 UTC) #15
TheMue
Please take a look. https://codereview.appspot.com/6853075/diff/8003/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode8 environ.go:8: var lxcDefaultFile = "/etc/default/lxc" On ...
6 years, 5 months ago (2013-01-31 15:54:11 UTC) #16
fwereade
Couple more remarks: https://codereview.appspot.com/6853075/diff/8003/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode8 environ.go:8: var lxcDefaultFile = "/etc/default/lxc" On 2013/01/31 ...
6 years, 5 months ago (2013-02-04 14:58:14 UTC) #17
TheMue
Please take a look. https://codereview.appspot.com/6853075/diff/18001/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/18001/golxc_test.go#newcode36 golxc_test.go:36: LXC_BRIDGE="lxcbr9" On 2013/02/04 14:58:14, fwereade ...
6 years, 5 months ago (2013-02-05 12:47:52 UTC) #18
fwereade
LGTM assuming complaints below are addressed. Var names and funky defers are very much suggestions, ...
6 years, 5 months ago (2013-02-07 09:43:11 UTC) #19
TheMue
6 years, 5 months ago (2013-02-07 11:39:18 UTC) #20
*** Submitted:

golxc: added configuration reading

Added the reading of configuration files, especially
the default configuration at /etc/default/lxc.

R=rog, fwereade, dfc, niemeyer
CC=
https://codereview.appspot.com/6853075

https://codereview.appspot.com/6853075/diff/25001/config.go
File config.go (right):

https://codereview.appspot.com/6853075/diff/25001/config.go#newcode11
config.go:11: bridgeRE string = `\n\s*LXC_BRIDGE="(\w+)"`
On 2013/02/07 09:43:11, fwereade wrote:
> Maybe MustCompile them up here, rather than waiting until ReadConf time to
> panic?

Done.

https://codereview.appspot.com/6853075/diff/25001/config.go#newcode22
config.go:22: addrs :=
regexp.MustCompile(addrRE).FindStringSubmatch(string(confData))
On 2013/02/07 09:43:11, fwereade wrote:
> nitpick var name -- I'd call this groups and stick the whole lot in its own
> scope, but YMMV. MustCompile should be extracted, though.

Done.

https://codereview.appspot.com/6853075/diff/25001/config.go#newcode26
config.go:26: bridges :=
regexp.MustCompile(bridgeRE).FindStringSubmatch(string(confData))
On 2013/02/07 09:43:11, fwereade wrote:
> ditto.

Done.

https://codereview.appspot.com/6853075/diff/25001/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6853075/diff/25001/golxc_test.go#newcode61
golxc_test.go:61: defer golxc.SetConfPath(orig)
On 2013/02/07 09:43:11, fwereade wrote:
> Hey, you could do:
> 
> defer golxc.SetConfPath(golxc.SetConfPath(cf))
> 
> ...but I accept that may not be to everybody's taste.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b