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.

Sign up for GitHub

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

Jump to bottom

Network aliases management #1000

Merged
merged 12 commits into from
May 16, 2018
Merged

Network aliases management #1000

merged 12 commits into from
May 16, 2018

Conversation

giafar
Copy link
Contributor

@giafar giafar commented Apr 15, 2018 •
edited
Loading

Hi all,
I had some problems to start an external docker-compose.yml because of network aliases configuration mismatch.
The DockerComposeServiceWrapper does not manage correctly the network name nor the aliases.
There is a unit test, the changes and a docker-compose file to check the behaviour.

Regards Gianluca.

version: '2.2'
services:
  service1:
    image: image
    networks:
      network1:
        aliases:
          - alias1
          - alias2
  service2:
    image: image
    networks:
      - network1
  service3:
    image: image
    networks:
      network1:
        aliases:
          - alias1

@codecov
Copy link

codecov bot commented Apr 15, 2018 •
edited
Loading

Codecov Report

Merging #1000 into master will increase coverage by 0.04%.
The diff coverage is 42.85%.

@@             Coverage Diff              @@
##             master    #1000      +/-   ##
============================================
+ Coverage      51.8%   51.85%   +0.04%     
- Complexity     1365     1393      +28     
============================================
  Files           147      147              
  Lines          7460     7548      +88     
  Branches       1132     1146      +14     
============================================
+ Hits           3865     3914      +49     
- Misses         3229     3263      +34     
- Partials        366      371       +5
Impacted Files Coverage Δ Complexity Δ
...g/handler/compose/DockerComposeServiceWrapper.java 58.76% <42.85%> (+4.99%) 55 <0> (+3) ⬆️
...ven/docker/config/handler/ImageConfigResolver.java 93.54% <0%> (-2.75%) 11% <0%> (+1%)
...ic8/maven/docker/config/RunImageConfiguration.java 91.48% <0%> (-0.88%) 47% <0%> (ø)
...va/io/fabric8/maven/docker/AbstractDockerMojo.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...ric8/maven/docker/service/DockerAccessFactory.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...c/main/java/io/fabric8/maven/docker/BuildMojo.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...rc/main/java/io/fabric8/maven/docker/SaveMojo.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...aven/docker/config/DockerMachineConfiguration.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...ig/handler/compose/DockerComposeConfigHandler.java 71.73% <0%> (ø) 17% <0%> (ø) ⬇️
...c/main/java/io/fabric8/maven/docker/StartMojo.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 14 more

rhuss
rhuss requested changes Apr 25, 2018
View reviewed changes
Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ! Could you please check your whitespace settings, the diff is by far too large. Looks like you've introduced tabs (we dont use tabs). I can see the really diff with the "No Whitespace" button, but then I can't comment on the PR.

Thanks !

giafar
giafar commented Apr 27, 2018
View reviewed changes
Copy link
Contributor Author

@giafar giafar left a comment

Choose a reason for hiding this comment

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

Changed tabs to spaces.
Sorry for my wrong settings ;-(

@rhuss
Copy link
Collaborator

rhuss commented Apr 27, 2018

Thanks ! but not sure what going on, as when I select https://github.com/fabric8io/docker-maven-plugin/pull/1000/files I still see that everything has changed. Could you try to squash your commits to one and force push it ?

thanks !

@giafar
Copy link
Contributor Author

giafar commented Apr 28, 2018

I don't know what is really going on: 0e05cc9

I'll squash everything

giafar
giafar commented May 4, 2018
View reviewed changes
Copy link
Contributor Author

@giafar giafar left a comment

Choose a reason for hiding this comment

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

Files committed with right settings.

@giafar
Copy link
Contributor Author

giafar commented May 15, 2018

@rhuss Is it possible to merge the PR ?
Thanks in advance.

rhuss
rhuss requested changes May 16, 2018
View reviewed changes
Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

thanks. looks good except that I would not use the implementation classes but the interfaces when looking up the aliases in various different ways.

src/main/java/io/fabric8/maven/docker/config/handler/compose/DockerComposeServiceWrapper.java Outdated
for (String alias : (List<String>) aliases) {
ret.addAlias(alias);
}
} else if (aliases instanceof LinkedHashMap) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't it be better to check just for Map ?

Copy link
Contributor Author

@giafar giafar May 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You are absolutely right

src/main/java/io/fabric8/maven/docker/config/handler/compose/DockerComposeServiceWrapper.java Outdated
ret.addAlias(alias);
}
} else if (aliases instanceof LinkedHashMap) {
LinkedHashMap<String, ArrayList<String>> map = (LinkedHashMap<String, ArrayList<String>>) aliases;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Map<String,List<String>> map because using the implementation classes as type parameters really causes a tight coupling and you really don't need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right again

src/main/java/io/fabric8/maven/docker/config/handler/compose/DockerComposeServiceWrapper.java Outdated
}
} else {
throwIllegalArgumentException(
"'networks:' Aliases must be given as a linked has map of strings. 'aliases' key not founded");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does it have to be a LinkedHashMap ? Just a "... as a map of strings", so I would use this error message:

'networks:' No aliases entry found in network config map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Message updated

rhuss
rhuss approved these changes May 16, 2018
View reviewed changes
Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

@rhuss rhuss merged commit 26e233b into fabric8io:master May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhuss rhuss rhuss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@giafar @rhuss