Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

networker API facade #16

Merged
merged 6 commits into from Jun 5, 2014
Merged

networker API facade #16

merged 6 commits into from Jun 5, 2014

Conversation

klyachin
Copy link

@klyachin klyachin commented Jun 4, 2014

api: new Networker API facade

The original task was:
"new Networker API facade (client/server) with ListVLANsForMachine API call"
The additional tas was:
"Networker API" should do all network setup staff done by cloud-init

But all thing are changing and now Networker API has the only method calld
MachineNetworkInfo(machineTag string) ([]network.Info, error).

IsVirtual bool
}

func (i *Info) ActualInterfaceName() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to bikeshed too much, but would these be better as RawInterfaceName and func InterfaceName() ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of this change is to make it easier to transition state-stored networks and interfaces for a machine into an API method result, keeping the same order and data in there. And this is enough for now for the networker to do its job. There were too many discussions on this already, let's not go back but forward with what we agreed upon please.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a doc comment to ActualInterfaceName, as it's exported.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jameinel
Copy link
Member

jameinel commented Jun 5, 2014

I think this is looking pretty good. I'd like to have Dimiter look it over, though.

John
=:->

c.Assert(s.networker, gc.NotNil)

// Expected results of MachineNetworkInfo for a machine and a containers
s.expectedMachineInfo = []network.Info{{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here? It should be in the actual test I think, not SetUpTest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite nice having it near the data that it needs to match, though. I'm still +1 on splitting this up, it might well help.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved all expectedResult struct to tests

return false
}
for parentId := state.ParentId(id); parentId != ""; parentId = state.ParentId(parentId) {
// Until reach top-level machine.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Until reach/Until we reach/ or Until a top-level machine is reached?

@dimitern
Copy link

dimitern commented Jun 5, 2014

Thanks, much better now - LGTM with just a few minor things to fix.

@klyachin
Copy link
Author

klyachin commented Jun 5, 2014

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Jun 5, 2014

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

@klyachin
Copy link
Author

klyachin commented Jun 5, 2014

$$merge$$

@klyachin
Copy link
Author

klyachin commented Jun 5, 2014

$$MERGE$$

jujubot added a commit that referenced this pull request Jun 5, 2014
api: new Networker API facade

The original task was:
"new Networker API facade (client/server) with ListVLANsForMachine API call"
The additional tas was:
"Networker API" should do all network setup staff done by cloud-init

But all thing are changing and now Networker API has the only method calld
MachineNetworkInfo(machineTag string) ([]network.Info, error).
@jujubot jujubot merged commit c5a50a4 into juju:master Jun 5, 2014
jujubot added a commit that referenced this pull request Jun 5, 2014
axw pushed a commit to axw/juju that referenced this pull request Jan 29, 2015
Demo code to set block devices in state
frankban pushed a commit to frankban/juju that referenced this pull request Apr 9, 2015
Patch juju#2049 diff from master (remove obsolete AddMetrics API)
ericsnowcurrently pushed a commit to ericsnowcurrently/juju that referenced this pull request May 26, 2015
Added "actions" location to wordpress test charm
mikemccracken pushed a commit to mikemccracken/juju that referenced this pull request Apr 13, 2017
state: add support for CAAS FindEntity and User.
wupeka pushed a commit to wupeka/juju that referenced this pull request Dec 17, 2017
Updated dependencies to enable extra-bindings from charm metadata to be honored for bundle deployments

No other changes than updating dependencies.tsv needed.
hmlanigan pushed a commit to hmlanigan/juju that referenced this pull request Aug 26, 2019
hmlanigan pushed a commit to hmlanigan/juju that referenced this pull request Aug 26, 2019
Add the method to start machines

Based on PRs juju#16 and juju#17.
laszlokajtar pushed a commit to laszlokajtar/juju that referenced this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants