Issue #6 : Add Namespace and Probes Configuration to Kubernetes Manifests #7
Conversation
There was a problem hiding this comment.
Thanks @RohanRusta21! Could you also add a description to the newly added fields
| } | ||
| } | ||
| }, | ||
| "readiness": { |
There was a problem hiding this comment.
Can you add order: ["path", "initialDelaySeconds"...]
There was a problem hiding this comment.
Hey @petar-cvit , Yup its done now. Please Review it :)
| - name: {{ $key }} | ||
| value: {{ $value | quote }} | ||
| {{- if .Values.general.environment }} | ||
| {{- toYaml .Values.general.environment | indent 12 }} |
There was a problem hiding this comment.
Can you revert this? With the UI for env variables, the indent doesn't work
| } | ||
| }, | ||
| "order": [ | ||
| "enabled", |
There was a problem hiding this comment.
can we add enabled to each probe separately? Also can you make it disabled by default in values.yaml
There was a problem hiding this comment.
Hi @petar-cvit , actually initially I tried to have separate toggle for each probe. But it was not working fine. Maybe will try it again.
There was a problem hiding this comment.
Please do. You can paste the error here if you want and we can try to figure it out
There was a problem hiding this comment.
Can you explain me this error.
UI changes performed :
There was a problem hiding this comment.
We refactored manifest rendering so you can try out the template again with the updated version.
| labels: | ||
| app: {{ .Values.name }} | ||
| spec: | ||
| replicas: {{ (.Values.scaling).replicas | default 1 }} |
There was a problem hiding this comment.
Can we leave this default?
| @@ -17,12 +18,14 @@ spec: | |||
| containers: | |||
| - name: {{ .Values.name }} | |||
| image: "{{ .Values.general.image }}:{{ .Values.general.version }}" | |||
| {{- if ((.Values.networking).expose) }} | |||
|
|
|||
There was a problem hiding this comment.
Could you remove all empty lines?
| ports: | ||
| - name: http | ||
| containerPort: {{ .Values.networking.port | default 80 }} | ||
| containerPort: {{ .Values.networking.port }} |
There was a problem hiding this comment.
What happens if port is not set? We cant enforce it since exposing is optional
There was a problem hiding this comment.
Yeah but this template is meant to expose the port specified by the user.
| resources: | ||
| {{- if ((.Values.scaling).resources).memory }} |
There was a problem hiding this comment.
Can you rever the resources object since its not format properly and it can cause a null pointer because of deleted brackets
| {{- end }} | ||
|
|
||
| {{- if .Values.probes.enabled }} |
There was a problem hiding this comment.
Can you add brackets the same way it was done in the resources object to avoid null objects
| } | ||
| }, | ||
| "order": [ | ||
| "enabled", |
There was a problem hiding this comment.
We refactored manifest rendering so you can try out the template again with the updated version.
|
@RohanRusta21, you also have some conflicts that need to be resolved. We renamed the chart since this PR was opened |
Hi @petar-cvit @KaradzaJuraj ,
This pull request focuses on the addition of new fields within various files, namely "deployment.yaml", "services.yaml", "values.schema.json", and "values.yaml". Here's a summary of the changes made:
In "demo-extended/templates/deployment.yaml", I have introduced YAML code for Namespace and Probes (specifically, readinessProbe, livenessProbe, and startupProbe). The default values for these fields are retrieved from "values.yaml".
Within "demo-extended/templates/services.yaml", I have incorporated YAML code for Namespace. Similar to the deployment template, the default values for these fields are sourced from "values.yaml".
In "demo-extended/values.schema.json", I have extended the JSON schema to include the Namespace field below Name, which falls under the metadata of deployment.yml. Additionally, I have structured a separate, expandable section for Probes, allowing users to conveniently review and customize each probe as needed.
Within "demo-extended/values.yaml", default values have been provided for the newly added fields in the branch, along with resources (CPU and memory), which were previously declared but lacked default values.
Please Review the PR and feel free to contact me if any improvement required from my end 👍 ,
Thanks & Regards,
Rohan Rustagi