/juju

network: fix quoting in bridgescript invocation #6758

Merged
merged 4 commits into from Jan 4, 2017

Conversation

Reviewers

@macgreagoir macgreagoir

@voidspace voidspace

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@frobware @macgreagoir @jujubot @voidspace @jameinel
@frobware
Contributor

frobware commented Jan 3, 2017

The deviceNames to bridge were being quoted as "[ens4 ens5]", when
they should have been just "ens4 ens5". This commit simplifies the
arguments passed to the invocation of the bridgescript.
network: fix quoting in bridgescript invocation 
The deviceNames to bridge were being quoted as "[ens4 ens5]", when
they should have been just "ens4 ens5". This commit simplifies the
arguments passed to the invocation of the bridgescript.
a3c8f3b
@macgreagoir
Contributor

macgreagoir commented Jan 3, 2017

LGTM. The Sprintf argument list is getting long, so a potential text/template candidate? I don't find it especially hard to follow, though.
@frobware
Contributor

frobware commented Jan 3, 2017

I need to EOD but will follow-up with unit tests.
@jameinel

jameinel approved these changes Jan 4, 2017
View changes

looks good, tests would be good, certainly.

@frobware
Contributor

frobware commented Jan 4, 2017

!!build!!
@macgreagoir

macgreagoir reviewed Jan 4, 2017
View changes

LGTM

One question around the use if the WallClock in tests.

@macgreagoir
Contributor

macgreagoir commented Jan 4, 2017

Just commenting to ack the new comment explaining the use of WallClock. Cheers!
@frobware
Contributor

frobware commented Jan 4, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jan 4, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 5cfc58e into juju:2.1-dynamic-bridges Jan 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment