Skip to content
This repository has been archived by the owner on Jun 16, 2021. It is now read-only.

Fixed error causing due to top level network key #467

Merged
merged 1 commit into from
May 15, 2017
Merged

Fixed error causing due to top level network key #467

merged 1 commit into from
May 15, 2017

Conversation

surajnarwade
Copy link
Contributor

Resolves #456

@surajnarwade
Copy link
Contributor Author

cc @vdemeester @joshwget

@surajnarwade surajnarwade mentioned this pull request May 15, 2017
Closed
vdemeester
vdemeester suggested changes May 15, 2017
View reviewed changes
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Thanks 👼
I would have fixed it a little differently :

if net, ok := p.NetworkConfigs[network.Name]; ok {
  if ret != nil & net.External.External {
    // […]
  }
}

This above should suffice to fix it, and remove duplication handling the name 😉

project/project.go Outdated
@@ -288,7 +288,8 @@ func (p *Project) handleNetworkConfig() {
// Consolidate the name of the network
// FIXME(vdemeester) probably shouldn't be there, maybe move that to interface/factory
for _, network := range serviceConfig.Networks.Networks {
if net, ok := p.NetworkConfigs[network.Name]; ok {
net, ok := p.NetworkConfigs[network.Name]
if ok == true && net != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if ok && net != nil 👼

project/project.go Outdated
network.RealName = p.Name + "_" + network.Name

p.NetworkConfigs[network.Name] = &config.NetworkConfig{
Driver: "bridge",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is required (the default driver is handled later on)

@surajnarwade
Copy link
Contributor Author

@vdemeester , it gives panic error for net.External.External , so I used this way

@surajnarwade
Fixed error causing due to top level network key
a478498 
Resolves docker#456

Signed-off-by: Suraj Narwade <surajnarwade353@gmail.com>
@surajnarwade
Copy link
Contributor Author

@vdemeester updated with new changes

vdemeester
vdemeester approved these changes May 15, 2017
View reviewed changes
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🦁

@vdemeester vdemeester merged commit 01ff892 into docker:master May 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vdemeester vdemeester vdemeester approved these changes

Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@surajnarwade @vdemeester @GordonTheTurtle