-
Notifications
You must be signed in to change notification settings - Fork 14
Triton support #17
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
Triton support #17
Conversation
… container names by test harness
| && tar zxf /tmp/${prom}.tar.gz -C /tmp \ | ||
| RUN export PROM_VER=1.5.2 \ | ||
| && export prom=prometheus-${PROM_VER}.linux-amd64 \ | ||
| && curl -Lso /tmp/prometheus.tar.gz https://github.com/prometheus/prometheus/releases/download/v${PROM_VER}/${prom}.tar.gz \ |
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.
Hm... should we open an issue with the Prometheus project to sign their releases?
| #!/bin/sh | ||
| set -e | ||
|
|
||
| if [ -f "${DOCKER_CERT_PATH}/ca.pem" ] |
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'm a little confused as to why we wouldn't be doing this in the preStart rather than in a wrapper around ContainerPilot?
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.
The specific case here is wrapping ContainerPilot, which means we lose PID 1 and we should probably always try to avoid, but there's a general question of when to wrap your app vs. use a preStart and what the tradeoffs might be.
Both of those points might deserve coverage in the docs @geek has been working on.
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 if you find that you really need to wrap ContainerPilot itself, then ContainerPilot has a bug or missing feature.
If you find that you really need to wrap the application rather than using a preStart, then ContainerPilot probably has a bug or missing feature (although realistically all the cases I've seen of this so far will be (have been) covered by the improved dependency handling in CPv3).
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.
The point here was to establish ENV vars accessible by all containerpilot scripts. Is there a better way?
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.
For now we'd typically set those in the setup script (as annoying as that is). It's a v3 feature to allow scripts to update the environment though.
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.
Ok. Will move it to setup.sh. Kirby was my inspiration for the current approach FWIW.
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.
Kirby was my inspiration for the current approach FWIW
That's fair. The difference between this and the referenced example is that the example was just a shell script, so ContainerPilot executed the shell script, it established its environment, and then all was good. Chaining that to a subsequent app is where it falls apart.
ContainerPilot 3 will have better support for changing the env vars via the control plane, but for now...
|
I'm going to carry this one and address the questions in #17 (comment) |
| image := $(namespace)/prometheus | ||
| testImage := $(namespace)/prometheus-testrunner | ||
|
|
||
| dockerLocal := DOCKER_HOST= DOCKER_TLS_VERIFY= DOCKER_CERT_PATH= 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.
This does not work on my Mac with Docker Machine
Replacing it with dockerLocal := docker does
| COPY examples/triton/docker-compose.yml /src/triton/docker-compose.yml | ||
|
|
||
| # install tests | ||
| COPY test/testing/testcases.py /src/testcases.py |
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.
File doesn't exist in repo
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.
That's a submoduled repo, so...
|
Closing in favor of #18 |
1.5.22.7.20.7.2Fixes #16, #8, and #5