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

[chart/navi-attractor] chart syncup #519

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

uk-navi-ci
Copy link
Collaborator

@uk-navi-ci uk-navi-ci commented Oct 10, 2024

Pull Request description

Changelog

  • part of navi-back functionality implemented as a separate service, required for DMA for Trucks

Breaking changes

  • none

Check-list. Чек-лист код-ревью

  • Запрос на слияние в develop.
  • Есть описание к PR.
  • Указаны блокирующие изменения. Breaking-Changes
  • Соответствие кода принятому стилю
    • Описание настроек.
    • Именование настроек.
    • Дефолтные значения.
    • Стиль кода.
  • Работоспособность. Разворачивается на своем окружении из ветки PR.
    • Тест API через тесты helmfile-хуков или коллекций Postman.
  • Не осталось мусора от удаления каких-то параметров. Ищется поиском по проекту из ветки PR.
  • Отработка линтера на чарт из ветки PR. Пример: helm lint charts/search-api

@i-bogomazov i-bogomazov force-pushed the auto/chart/navi-attractor branch 2 times, most recently from 7f38062 to 453ce5d Compare October 21, 2024 03:34
@i-bogomazov i-bogomazov changed the title WIP: [chart/navi-attractor] chart syncup [chart/navi-attractor] chart syncup Oct 30, 2024
@i-bogomazov i-bogomazov marked this pull request as ready for review October 30, 2024 07:32
@i-bogomazov i-bogomazov requested review from a team as code owners October 30, 2024 07:32
@dbelyaev-nsk
Copy link
Contributor

{{- if .Values.s3.suffix }}
"suffix": {{ .Values.s3.suffix | quote }},
{{- end }}
"endpoint": {{ .Values.s3.host | quote }},
Copy link
Contributor

Choose a reason for hiding this comment

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

Отсутствует required для обязательных параметров S3 если он включен в настройках.

annotations:
{{- include "tplvalues.render" ( dict "value" .Values.service.annotations "context" $) | nindent 4 }}
{{- end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Лишняя пустая строка.

{{- if .Values.service.http.annotations }}
{{- include "tplvalues.render" ( dict "value" .Values.service.http.annotations "context" $) | nindent 4 }}
{{- end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Лишняя пустая строка.

labels:
{{- include "generic-chart.labels" . | nindent 4 }}
rule: {{ .Values.attractor.app_rule | default "" | quote }}
navigroup: {{ .Values.navigroup | default "" | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Описание переменной отсутствует в values.yaml

name: {{ include "generic-chart.fullname" . }}
labels:
{{- include "generic-chart.labels" . | nindent 4 }}
rule: {{ .Values.attractor.app_rule | default "" | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Описание переменной отсутствует в values.yaml

{{- end }}{{- /* .Values.attractor.indices.geoImport.enabled */}}


{{- if eq .Values.attractor.type "truck" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Описание переменной отсутствует в values.yaml

{{- end -}}
{{ $rules | toPrettyJson | nindent 6 -}}
{{- end -}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Разное число пустых строк между блоками. Где-то одна, где-то две.
Предлагаю привести к везде к одной.

Override generic-chart
TODO: rewrite https://github.com/helm/helm/issues/11291
*/}}
{{- define "generic-chart.containerName" -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Объявление нового шаблона с именем из чужого домена.
При чтении чарта не понятно где её искать. В этом чарте или в чарте generic-chart


# @section Frozen data settings

# @param frozenData.enabled If use frozen data is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Возможно стоит скипнуть переменные frozenData. Вряд-ли они где-то востребованы.

# @param attractor.rtr.enabled Enable real time restrictions.
# @param attractor.rtr.url URL real time restrictions server.
# @param attractor.rtr.updatePeriod Update period from real time restrictions server.
# @param attractor.mock If mock service being deployed. For test purposes only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Возможно стоит скипнуть эту настройку.

# @param ingress.enabled If Ingress is enabled for the service.
# @param ingress.hosts[0].host Hostname for the Ingress service.

ingress:
Copy link
Contributor

Choose a reason for hiding this comment

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

Описание ingress нужно расширить по аналогии, иначе не понятно куда и как определять секреты для tls.

# @param resources.limits.cpu [nullable] CPU limit, recommended value `3000m`.
# @param resources.limits.memory [nullable] Memory limit, recommended value `8Gi`.

resources: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Тут вопрос. Если выше написаны рекомендованные значение, то почему бы не перенести их сюда?

# @extra vpa.maxAllowed.cpu Upper limit for the number of CPUs to which the autoscaler can scale up.
# @extra vpa.maxAllowed.memory Upper limit for the RAM size to which the autoscaler can scale up.

vpa:
Copy link
Contributor

Choose a reason for hiding this comment

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

Описание vpa нужно расширить по аналогии.

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.

3 participants