search

open-sauced / pizza

This is an engine that sources git commits and turns them to insights
Apache License 2.0
31 stars 13 forks source link

Align Kubernetes pod spec command with Dockerfile CMD #13

Closed MartiinWalsh closed 1 year ago

MartiinWalsh commented 1 year ago

What type of PR is this? (check all applicable)

Description

This PR resolves issue #12, where the command field in the Kubernetes pod spec was not aligned with the CMD instruction in the Dockerfile.

The problem led to an error on container startup, as the container was looking for the pizza-oven executable in the wrong location (./pizza-oven instead of /usr/bin/pizza-oven).

Instead of aligning the command path with the Dockerfile's CMD, this PR removes the command field from the pod spec entirely. This change will cause Kubernetes to default to the command specified in the Dockerfile (CMD ["/usr/bin/pizza-oven"]), ensuring the container is able to locate and execute the pizza-oven binary correctly on startup.

Here is the updated pod spec:

spec:
  containers:
  - name: pizza-oven
    image: pizza-oven:latest
    imagePullPolicy: IfNotPresent

  1. Docker Linting: An error was thrown when running `make lint because Docker expected an absolute path for the host directory, but a relative path was provided. The command has been updated to provide the absolute path, i.e., -v ./:/app has been changed to -v /usr/bin/pizza-oven:/app .

  2. Local Kubernetes Setup: Added Helm to the list of requirements for setting up the application locally with Kubernetes.

Related Tickets & Documents

Fixes #12

Mobile & Desktop Screenshots/Recordings

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

MartiinWalsh commented 1 year ago

Although I have opted to remove the command field from the Kubernetes pod spec to default to the Dockerfile's CMD instruction, I understand that there may be a preference for explicitness in defining behaviour in the Kubernetes configuration, rather than relying on defaults.

If the goal is to maintain explicitness, we could instead modify the command field to align with the Dockerfile, like so:

spec:
  containers:
  - name: pizza-oven
    image: pizza-oven:latest
    imagePullPolicy: IfNotPresent
    command: ["/usr/bin/pizza-oven"]

I am more than happy to revise this PR to reflect that approach, if preferred. I welcome any feedback on the best way to proceed.

jpmcb commented 1 year ago

Welcome!! 🍕 Thanks for the first time contribution!!!

TrojanDeveloper commented 1 year ago

Okay, you're right, I'll review correctly in the future. Thank you for your help.

MartiinWalsh commented 1 year ago

I have updated this PR as per your suggestions, @jpmcb . Specifically, I've modified the Makefile to now utilise an absolute path when defining a Docker volume. Additionally, I decided to reintroduce the command field to the pod specification. This field now points to the updated path instead of defaulting to the Dockerfile's CMD instruction. I believe this explicit approach improves readability and minimises potential oversight.

jpmcb commented 1 year ago

Great work @MartiinWalsh!! Great first contribution! 👏🏼

MartiinWalsh commented 1 year ago

Thank you, I appreciate the feedback!