-
Notifications
You must be signed in to change notification settings - Fork 92
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
Create data directories #4
Conversation
Create data directories automatically. * Add flag to disable automatic creation.
3c2d086
to
d7538b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thank you very much for your awesome contribution.
I just left a minor comment. :) But everything LGTM
.travis.yml
Outdated
@@ -13,15 +13,15 @@ services: | |||
# Ensure docker is updated | |||
before_install: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can actually remove this and just use
# Enable the docker service
services:
- docker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, the only thing it does now is upgrade docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add cache: pip
to cache packages installed via pip
* Adds yamllint. * Use working docker images.
molecule/default/molecule.yml
Outdated
@@ -10,12 +10,28 @@ platforms: | |||
privileged: true | |||
volumes: | |||
- /sys/fs/cgroup:/sys/fs/cgroup:ro | |||
- name: minio-fedora-27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we depend on externally build docker images?
Can't something like that be implemented?
I know that it's not super efficient, but it makes the CI self-contained within the role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test time efficiency was the goal. @paulfantom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I use those images to reduce time needed for CI pipeline. Those are automatically built from this repo https://github.com/paulfantom/dockerfiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atosatto self-contained images are ok, but consider testing this across 3 versions of ansible and 5-6 distros (our basic test matrix, used for example here: https://github.com/cloudalchemy/ansible-prometheus), this will cause to build those images 15-18 times for every new commit. This takes a lot of time and doesn't scale well. Also this way when something breaks in one repo, we can quickly fix it for all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the same in the past (https://github.com/atosatto/dockerfiles) and then switched to self-contained images built as part of the CI project.
I am fine with the change for now. We can always switch back to Docker images built as part of the CI in a next stage.
travis: switch to python 3, test ansible 2.9
Create data directories automatically.