Conversation
These examples uses the image as defined here openSUSE/docker-containers#47, which contains the latest master. Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
See SUSE#1254 Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
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 in the process of testing this out and had a few suggestions plus a few things that might be incorrect here. Awesome work, thanks!
capacity: | ||
storage: 20Gi | ||
accessModes: | ||
- ReadWriteMany |
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 not ReadWriteOnce? Same thing for other uses of ReadWriteMany
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.
Yeah, this is something to improve (I just blindly gave access to everything to make sure that it works 😁)
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
name: mariadb-secrets |
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 the naming of all the objects it would be nice to prefix them with "portus" to avoid conflicts with existing things (and to keep all the portus stuff grouped together visually in kubectl and dashboard)
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.
On second thought, it might be nicer to have the example put everything in a portus
namespace, since that's a cleaner way to run it anyway
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.
Sounds good 👍
# type: LoadBalancer | ||
ports: | ||
- port: 3000 | ||
targetPort: 3000 |
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.
How does this port tie to the container port? It's not clear from this manifest
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.
Maybe I should add documentation on this. It's stated on the official Docker image of Portus though.
containers: | ||
- name: registry | ||
image: library/registry:2.6 | ||
command: ["/bin/sh", "/etc/docker/registry/init"] |
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.
Rather than having a custom startup script, would it make sense to move the configuration to either environment variables (docker registry supports all configuration via env), or a configmap?
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.
Not quite. This init script will also install self-signed certificates. Something that we wouldn't have on a serious deployment, but that it's needed in simple deployments that are testing things out. Maybe I should add a comment on that as well.
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, since making that comment I figured out that the certs were needed. Rather than doing it with an init script baked into the image, I ended up writing a simple helper script to create the certs and add them as a secret, then mounted the secret to both portus and registry. That makes it slightly more visible. A comment on what the script is doing and when you'd need or not need it would be very helpful.
volumes: | ||
- name: secrets | ||
secret: | ||
secretName: certificates |
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 like this should be portus-secrets, not certificates
cpu: 100m | ||
memory: 200Mi | ||
ports: | ||
- containerPort: 443 |
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 443 here should be 5000 instead?
- ReadWriteMany | ||
resources: | ||
requests: | ||
storage: 1Gi |
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.
Shouldn't this match the 10Gi
for the PersistentVolume
above?
do note the Helm Chart which simplifies deployment on k8s - helm/charts#1284 and includes conditional support to install mariadb / minio (as well as create the bucket automagically) - let me know if you want to take over maintenance of that PR It has diverged too much from our internal deployment (we do not need mariadb and minio... as we use Terraform to provision AWS RDS instances and S3 buckets) |
Closing this in favor of this Helm chart which will eventually be pushed into the official kubernetes repo. |
This should be an example that anyone could test, regardless of deployment method. I still haven't had the time to fully complete it, but I don't want it to block the bulk of #1254. So, I'm moving this part into this PR.