search

saltstack-formulas / docker-formula

Install and set up Docker
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
136 stars 330 forks source link

fix(compose-ng): Adding ability to set devices #269

Closed japtain-cack closed 3 years ago

japtain-cack commented 3 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

This adds support for devices. List of host devices to expose within the container. Can be expressed as a comma-separated list or a YAML list.

Pillar / config required to test the proposed changes

docker:
  compose:
    my-image:
      image: my/image
      devices:
        - "/dev/snd"

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

noelmcloughlin commented 3 years ago

LGTM, but can you add example pillars to pillar.example file. And update commit message to say "feat(compose-ng): devices support" so that Lint check passes. thanks

japtain-cack commented 3 years ago

Yeah, and actually I jumped the gun on the PR. There is an issue. I'll fix tomorrow though.

japtain-cack commented 3 years ago

Should I add a new container as an example or add the devices key to an existing one?

noelmcloughlin commented 3 years ago

Whichever you prefer to be honest. thanks

On Thu, Dec 17, 2020 at 9:48 PM Nathan Snow notifications@github.com wrote:

Should I add a new container as an example or add the devices key to an existing one?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/saltstack-formulas/docker-formula/pull/269#issuecomment-747722180, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFUUQVAQUX5U3O7IRSYKWLSVJ4CNANCNFSM4U7HE6TA .

actual-nsnow commented 3 years ago

Added pillar example

japtain-cack commented 3 years ago

Oops, wrong account. That was my comment just above.

myii commented 3 years ago

@japtain-cack The CI is finally running again and it's showing us that this is failing the pre-commit checks, specifically for yamllint.

https://gitlab.com/saltstack-formulas/docker-formula/-/jobs/921902322#L158

Check YAML syntax with yamllint..........................................Failed
- hook id: yamllint
- duration: 0.89s
- exit code: 2
./pillar.example
  167:14    warning  truthy value should be one of [false, true]  (truthy)
  168:21    warning  truthy value should be one of [false, true]  (truthy)
./test/salt/pillar/archive.sls
  167:14    warning  truthy value should be one of [false, true]  (truthy)
  168:21    warning  truthy value should be one of [false, true]  (truthy)

While you haven't touched ./test/salt/pillar/archive.sls in this PR, it probably squeezed through when Travis CI stopped working. Would you mind adding that fix to this PR as well?

noelmcloughlin commented 3 years ago

I merged https://github.com/saltstack-formulas/docker-formula/pull/270, thanks. Please get GitLab CI (funny to say that) working and all should be good. I doubt CI supports arm64?

japtain-cack commented 3 years ago

Ok, I think I have all the tests passing now. Let me know if I should do anything else.

myii commented 3 years ago

Ok, I think I have all the tests passing now. Let me know if I should do anything else.

Thanks @japtain-cack, CI tests passing so all looks good -- merged.

I merged #270, thanks. Please get GitLab CI (funny to say that) working and all should be good. I doubt CI supports arm64?

@noelmcloughlin This formulas was one of 4 out of 95 that I couldn't get working properly in GitLab CI. We're currently only running all of the pre-flight checks. Perhaps the arm64 has something to do with it? In any case, there is some work being done with GitHub Actions as well, so we can explore that avenue at some time in the future, hopefully.

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 1.1.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

noelmcloughlin commented 3 years ago

@myii 91/95 is impressive. well done. Some people are running this formula on Rasberry Pi so #270 was arch64. I remember fixing up Travis CI on this formula a while back and wondering if the platform should be changed to vagrant so that containers and swarm and compose could be tested properly. But I done what I could on driver: docker. Does docker run on docker?

myii commented 3 years ago

I remember fixing up Travis CI on this formula a while back and wondering if the platform should be changed to vagrant so that containers and swarm and compose could be tested properly.

@noelmcloughlin Vagrant-based testing (using GitHub Actions) is probably the way to go for this formula. We've got some (Windows) examples:

Obviously, would need to use Linux platforms!

But I done what I could on driver: docker. Does docker run on docker?

Yes, we're using dind (Docker-in-Docker) in GitLab CI:

https://github.com/saltstack-formulas/docker-formula/blob/a5b95c01377db3ab9f63210234ac19aa51043c88/.gitlab-ci.yml#L25

The problem is that this formula would require Docker-in-Docker-in-Docker(!), which I wouldn't expect to work too well:

To be honest, even Travis CI was struggling with that and not all of the states could be run in that setup.