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.

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

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

Merged
merged 1 commit into from
Dec 18, 2020

Conversation

japtain-cack
Copy link
Contributor

@japtain-cack japtain-cack commented Dec 17, 2020

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

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

  • 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

@noelmcloughlin
Copy link
Member

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
Copy link
Contributor Author

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

@japtain-cack japtain-cack force-pushed the nsnow/devices branch 4 times, most recently from 96a0ba7 to 2f9e0e6 Compare December 17, 2020 21:38
@japtain-cack
Copy link
Contributor Author

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

@noelmcloughlin
Copy link
Member

noelmcloughlin commented Dec 17, 2020 via email

@actual-nsnow
Copy link

Added pillar example

@japtain-cack
Copy link
Contributor Author

japtain-cack commented Dec 18, 2020

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

@myii
Copy link
Member

myii commented Dec 18, 2020

@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)
  • Basically, True and False should use their lowercase equivalents.

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
Copy link
Member

noelmcloughlin commented Dec 18, 2020

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

@japtain-cack
Copy link
Contributor Author

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

@myii myii merged commit b739204 into saltstack-formulas:master Dec 18, 2020
@myii
Copy link
Member

myii commented Dec 18, 2020

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

🎉 This PR is included in version 1.1.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@noelmcloughlin
Copy link
Member

@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
Copy link
Member

myii commented Dec 19, 2020

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:

- 'docker:dind'

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

  • myii/ssf-dind-ruby:2.7.1-r3 -- a customised dind image (with Ruby installed) to run our pre-salted images in
    • The pre-salted image itself (e.g. saltimages/salt-master-py3:debian-10) -- spun up by Kitchen
      • The Docker installed and run by this formula

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants