feat(compose-ng): Added networks keyword support

[olipinski]

PR progress checklist (to be filled in by reviewers)

• Changes to documentation are appropriate (or tick if not required)
• Changes to tests are appropriate (or tick if not required)
• Reviews completed

---

What type of PR is this?

Primary type

• [build] Changes related to the build system
• [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
• [ci] Changes to the continuous integration configuration
• [feat] A new feature
• [fix] A bug fix
• [perf] A code change that improves performance
• [refactor] A code change that neither fixes a bug nor adds a feature
• [revert] A change used to revert a previous commit
• [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

• [docs] Documentation changes
• [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

None as far as I could find.

Describe the changes you're proposing

I've added the support for the network keyword. So far it's just that with no support for aliases. I have also commented out links from pillar.example as it is a legacy feature that may be removed in future versions of docker.

Pillar / config required to test the proposed changes

Pillar.example is updated with the proposed change.

Debug log showing how the proposed changes work

`

      ID: proxy network
Function: docker_network.present
    Name: proxy
  Result: True
 Comment: Network 'proxy' already exists, and is configured as specified
 Started: 15:24:30.074586
Duration: 492.139 ms
 Changes:   

---

      ID: proxy
Function: docker_image.present
    Name: nginx
  Result: True
 Comment: Image nginx:latest already present
 Started: 15:24:30.567083
Duration: 7.594 ms
 Changes:   

---

      ID: proxy container
Function: docker_container.running
    Name: proxy
  Result: True
 Comment: Container 'proxy' is already configured as specified. Connected to network 'proxy'.
 Started: 15:24:30.575423
Duration: 1852.23 ms
 Changes:   
          ----------
          container:
              ----------
              Networks:
                  ----------
                  proxy:
                      ----------
                      Gateway:
                          ----------
                          new:
                              172.18.0.1
                          old:
                              None
                      IPAddress:
                          ----------
                          new:
                              172.18.0.2
                          old:
                              None

`

Documentation checklist

• Updated the README (e.g. Available states).
• Updated pillar.example.

Testing checklist

• Included in Kitchen (i.e. under state_top).
• Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
• Updated the relevant test pillar.

Additional context

[pull-assistant]

Best reviewed: commit by commit

---

Optimal code review plan

     feat(compose-ng): added networks keyword support

Powered by Pull Assistant. Last update 9635d6a ... 9635d6a. Read the comment docs.

[myii]

Apologies for the delayed reponse, @Exoyds. Unfortunately, there's no dedicated maintainer for this formula so things can take some time. Let's evaluate the situation for getting this merged:

1. Most importantly, it's exceptionally helpful to get a review from someone using this formula. From the networks graph, I can see that @kiniou @zekker6 have got recent versions of the formula. Could either of you provide a comment about this PR?
2. Some of the platforms are failing in Travis, which are unrelated to this PR -- that's something that I should be able to help with a bit further along.
3. This PR is getting caught by the commitlint -- please see the contributing guidelines linked below. Worst case, the commit message can be fixed when merging this.

[kiniou]

I'm not using compose-ng (i prefer using docker-containers instead) from this formula but I can take a look.

I think there is indeed a need to add some docker network states and this could be a valuable addition that could be shared across compose-ng and docker-containers.

[myii]

I'm not using compose-ng (i prefer using docker-containers instead) from this formula but I can take a look.

Really appreciate it, @kiniou.

[olipinski]

Thank you for looking into my PR! I completely understand the response time, after all we're contributing in our free time!

As for the PR itself - as I have mentioned I have tested it myself on a small environment, but on nothing bigger, so if there are any issues on larger deployments I can try and fix them

3. This PR is getting caught by the `commitlint` -- please see the contributing guidelines linked below.  Worst case, the commit message can be fixed when merging this.

This was because I missed a colon in the title when i initially submitted the PR, and I am not sure how to re-run the test.

[myii]

This was because I missed a colon in the title when i initially submitted the PR, ...

@Exoyds Also the capital A. So you'll want:

-feat(compose-ng) Added networks keyword support
+feat(compose-ng): added networks keyword support

... and I am not sure how to re-run the test.

You can amend the commit at your end and then force-push to your branch (which is your master branch, looking above). BTW, it's usually best to avoid using your master branch for pull requests. I can link to some reference materials later, when I get a chance.

[olipinski]

I've fixed all the commitlint issues, but the Travis one persists.

Also I would very much appreciate any materials and pointers as I'm relatively new to contributing to other projects!

[myii]

I've fixed all the commitlint issues, but the Travis one persists.

Also I would very much appreciate any materials and pointers as I'm relatively new to contributing to other projects!

@Exoyds Not to worry, you've done everything necessary from your end -- thanks for that. We need to upgrade to the latest CI images and that will get everything passing in Travis again. So now we're just waiting for any feedback that @kiniou has.

[kiniou]

Hi @Exoyds 👋 (and sorry for the late review: back-to-work and life combo happened)

👍 LGTM and I just left a small comment about splitting the networks creation from the docker.compose-ng SLS.
All in all, everything went well. I've tested your changes with a small Debian Buster machine with salt 3001 with the following pillar (not to far from pillar.example):

docker:
  networks:
    - nginxnet

  compose:
    nginx:
      image: nginx:latest
      container_name: nginx
      networks:
        - nginxnet
      ports:
        - '80:80'
        - '443:443'

    mailhog:
      image: mailhog/mailhog:latest
      networks:
        - nginxnet
      ports:
        - '8025:8025'

and I can see the containers attached to the network nginxnet:

docker network inspect --format '{{json .Containers}}' nginxnet | jq .
{
  "436b48d8a97513b346f31de96c3bc0c0ec397672518af91b7a263d452d6d1467": {
    "Name": "nginx",
    "EndpointID": "a85b966910a375a3e3b47091b2de8a8d29a790aadb18b6bdf8c2e3d873c8ba08",
    "MacAddress": "02:42:ac:12:00:03",
    "IPv4Address": "172.18.0.3/16",
    "IPv6Address": ""
  },
  "a761cb61488de19c73eff579536f68bc907973c413e8e1ffee65a117108be86c": {
    "Name": "mailhog",
    "EndpointID": "9b9c4065e19f78817ffa2f67e3960cbc561e23a51e54cdd199f158a9b2e0c924",
    "MacAddress": "02:42:ac:12:00:02",
    "IPv4Address": "172.18.0.2/16",
    "IPv6Address": ""
  }
}

[kiniou]

I think this should go in a separate networks.sls and just added to the list of include:.

This will allow docker.containers to benefit from that (not related but can be done in another PR).

Also, there are other kinds of docker networks and this would be easier to handle them in a separate file.

[olipinski]

I have now swapped it to a different file and added to networks.sls. It is indeed a good idea for future expansions!

[olipinski]

Hi @kiniou, no worries, and thank you for your time!

I'm glad to hear it worked! I have also made the changes as suggested in the review.

[noelmcloughlin]

Thanks for the PR! I have included this in #256 but the only difference is docker/compose/networks.sls location. So hopefully my PR gets merged (Travis is passing) and networks can be evolved in future PR as discussed here.

[olipinski]

Thanks for the PR! I have included this in #256 but the only difference is docker/compose/networks.sls location. So hopefully my PR gets merged (Travis is passing) and networks can be evolved in future PR as discussed here.

Thank you for including my PR in the code! Should I close this one now?

[noelmcloughlin]

Thanks @Exoyds for the feedback, Yes, I will close this now.

I actually updated the commit reverting to your design, so its docker/networks.sls is implmented per your PR.