feat(compose-ng): Added networks keyword support

olipinski commented 4 years ago

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
[x] [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).
[x] 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[bot] commented 4 years ago

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 commented 4 years ago

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:

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?
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.
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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

olipinski commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

saltstack-formulas / docker-formula