Skip to content
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

Solution:1.0 #2

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Solution:1.0 #2

wants to merge 10 commits into from

Conversation

cth-usq
Copy link

@cth-usq cth-usq commented Apr 27, 2024

No description provided.

Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general good job, bold and deep parametrization on first review iteration.
Still needs polishing and fixing some minor issues described via comments.

Comment on lines 5 to 23
# A chart can be either an 'application' or a 'library' chart.
#
# Application charts are a collection of templates that can be packaged into versioned archives
# to be deployed.
#
# Library charts provide useful utilities or functions for the chart developer. They're included as
# a dependency of application charts to inject those utilities and functions into the rendering
# pipeline. Library charts do not define any templates and therefore cannot be deployed.
type: application

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.0

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove generated comments, they make configs look messy.

averageUtilization: 70

secret:
SECRET_KEY: QGUyKHl4KXYmdGdoM19zPTB5amEtaSFkcGVieHN6XmRnNDd4KS1rJmtxXzN6Zio5ZSoK(@e2(yx)v&tgh3_s=0yja-i!dpebxsz^dg47x)-k&kq_3zf*9e*)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not store secret value as a plain text in values.yml

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question to this part.
In video "Creating Helm Chart - Working with values files" (27 sec), mentor implemented it in exect way.
Could you say how it must be implemented? I need to use values in b64?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or create a 3rd file with secretKeyRef on secret.yml. and that values copy to values.yaml, after paste to deployment.yml?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cth-usq good question. I will repeat this question to course creators.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +43 to +48
DB_USER: "app_user"
DB_PASSWORD: "1234"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not store secret value as a plain text in values.yml

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secrets wrongly configured and no configs for ServiceAccount

@@ -0,0 +1,51 @@
common:
namespace: todoapp

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Namespace is not common for subchart

Comment on lines 21 to 20
MYSQL_ROOT_PASSWORD: 1234
MYSQL_USER: app_user
MYSQL_PASSWORD: 1234

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with secrets here.

Comment on lines 5 to 23
# A chart can be either an 'application' or a 'library' chart.
#
# Application charts are a collection of templates that can be packaged into versioned archives
# to be deployed.
#
# Library charts provide useful utilities or functions for the chart developer. They're included as
# a dependency of application charts to inject those utilities and functions into the rendering
# pipeline. Library charts do not define any templates and therefore cannot be deployed.
type: application

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.0

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove generated text here as well.

Comment on lines 15 to 17
# pod-name.service-name.namespace.svc.cluster.local
# pod-name.service-name
# mysql-0.mysql.mysql.svc.cluster.local

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented code.

requests:
cpu: {{ .Values.mysql.statefulSet.resources.requests.cpu }}
memory: {{ .Values.mysql.statefulSet.resources.requests.memory }}
livenessProbe:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could import the whole block from values. And same for other below

Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes requested.
Щодо secrets, на цьому етапі зараховуємо завдання з секретами у values.yml, як це. було показано на відео.

apiVersion: v1
kind: ConfigMap
metadata:
name: {{ .Chart.Name }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use chart name as prefix, not a full name:

{{ .Chart.Name }}-configmap

apiVersion: v1
kind: Service
metadata:
name: {{ .Chart.Name }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: {{ .Chart.Name }}
name: {{ .Chart.Name }}-clusterip

apiVersion: apps/v1
kind: StatefulSet
metadata:
name: {{ .Chart.Name }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: {{ .Chart.Name }}
name: {{ .Chart.Name }}-statefulset

Comment on lines 39 to 44
requests:
{{ .Values.mysql.statefulSet.resources.requests }}
livenessProbe:
{{ .Values.mysql.statefulSet.livenessProbe }}
readinessProbe:
{{ .Values.mysql.statefulSet.readinessProbe }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not changed.

Copy link
Author

@cth-usq cth-usq May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean I need to unite resources, levenessProbe and readinesP - and import by one link?

because I made changes from previous commit

image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, resources, livenessProbe and readinesS on the same level in code line maybe will be better to keep them as 3 lines

Comment on lines +33 to +39
resources:
requests:
memory: {{ .Values.todoapp.resources.requests.memory | quote }}
cpu: {{ .Values.todoapp.resources.requests.cpu | quote }}
limits:
memory: {{ .Values.todoapp.resources.limits.memory | quote }}
cpu: {{ .Values.todoapp.resources.limits.cpu | quote }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be imported whole block.

bootstrap.sh Outdated

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete commented lines.

@cth-usq
Copy link
Author

cth-usq commented May 1, 2024

Minor changes requested. Щодо secrets, на цьому етапі зараховуємо завдання з секретами у values.yml, як це. було показано на відео.

@YuriiSmolii Тобто так як я зробив відразу?

@cth-usq cth-usq requested a review from YuriiSmolii May 1, 2024 10:47
@YuriiSmolii
Copy link

Minor changes requested. Щодо secrets, на цьому етапі зараховуємо завдання з секретами у values.yml, як це. було показано на відео.

@YuriiSmolii Тобто так як я зробив відразу?

Так

Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change requested.

limits:
memory: {{ .Values.todoapp.resources.limits.memory | quote }}
cpu: {{ .Values.todoapp.resources.limits.cpu | quote }}
{{ .Values.todoapp.resources.requests.memory }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут неправильно імпортований блок, потрібно не лише memory, а весь блок resource конфігурацій.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Приклад:

    spec:
      containers:
        - name: my-container
          image: my-image:latest
          resources:
            {{ toYaml .Values.resources | nindent 12 }}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YuriiSmolii my inattention

@cth-usq cth-usq requested a review from YuriiSmolii May 1, 2024 14:31
Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix lint error below and use "verify --debug" command to double-check solution.

image

@cth-usq
Copy link
Author

cth-usq commented May 2, 2024

Fix lint error below and use "verify --debug" command to double-check solution.

image

Looks like fixed

image

@cth-usq cth-usq requested a review from YuriiSmolii May 2, 2024 13:05
Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все ще багато помилок, ось як виправити:
image
image
image
image

Дана команда повинна проходити успішно:
helm template . --validate --debug

@cth-usq cth-usq requested a review from YuriiSmolii May 3, 2024 13:19
Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Виправлено неправильно, команда, яку я вказав у попередній перевірці
успішно не проходить

@cth-usq
Copy link
Author

cth-usq commented May 9, 2024

@YuriiSmolii

helm template . --validate --debug та helm lint- роходять успішно
image

Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Рухаємось далі.

  1. Все ж так як було сказано на QnA секрети стфорються з неправильним ключем-значенням.
  2. Потрібно ще виправити кілька помилок щоб успішно задеплоїти чарт наступною командою : helm install todoapp .
image

@cth-usq cth-usq requested a review from YuriiSmolii May 15, 2024 10:52
@cth-usq
Copy link
Author

cth-usq commented May 15, 2024

Рухаємось далі.

  1. Все ж так як було сказано на QnA секрети стфорються з неправильним ключем-значенням.
  2. Потрібно ще виправити кілька помилок щоб успішно задеплоїти чарт наступною командою : helm install todoapp .
image

Рухаємось далі.

  1. Все ж так як було сказано на QnA секрети стфорються з неправильним ключем-значенням.
  2. Потрібно ще виправити кілька помилок щоб успішно задеплоїти чарт наступною командою : helm install todoapp .
image

@YuriiSmolii

Все виправив Helm запускається
image

Copy link

@YuriiSmolii YuriiSmolii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

LevAndrii added a commit to LevAndrii/devops_todolist_kubernetes_task_12_helm_charts that referenced this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants