cvat icon indicating copy to clipboard operation
cvat copied to clipboard

[GSoC2024] update helm chart: Add common sub-chart

Open abhi-bhatra opened this issue 11 months ago • 12 comments

Motivation and context

There has been multiple issue coming due to conflict in common chart version of Redis and Postgresql. Fixes #7449 Fixes #7507

How has this been tested?

  1. Apply Helm chart with analytics enabled (Result: helm chart working)
  2. Now, try to run helm chart again, with analytics disabled (Result: helm chart will get an error) Below is the error:
Error: template: cvat/charts/redis/templates/master/service.yaml:12:14: executing "cvat/charts/redis/templates/master/service.yaml" at <include "common.labels.standard" (dict "customLabels" .Values.commonLabels "context" $)>: error calling include: template: cvat/charts/postgresql/charts/common/templates/_labels.tpl:6:27: executing "common.labels.standard" at <include "common.names.name" .>: error calling include: template: cvat/charts/postgresql/charts/common/templates/_names.tpl:6:18: executing "common.names.name" at <.Chart.Name>: nil pointer evaluating interface {}.Name
helm.go:84: [debug] template: cvat/charts/redis/templates/master/service.yaml:12:14: executing "cvat/charts/redis/templates/master/service.yaml" at <include "common.labels.standard" (dict "customLabels" .Values.commonLabels "context" $)>: error calling include: template: cvat/charts/postgresql/charts/common/templates/_labels.tpl:6:27: executing "common.labels.standard" at <include "common.names.name" .>: error calling include: template: cvat/charts/postgresql/charts/common/templates/_names.tpl:6:18: executing "common.names.name" at <.Chart.Name>: nil pointer evaluating interface {}.Name

Checklist

  • [x] I submit my changes into the develop branch
  • [ ] I have created a changelog fragment
  • [x] I have updated the documentation accordingly
  • [x] I have added tests to cover my changes
  • [x] I have linked related issues (see GitHub docs)
  • [x] I have increased versions of npm packages if it is necessary (cvat-canvas, cvat-core, cvat-data and cvat-ui)

License

  • [x] I submit my code changes under the same MIT License that covers the project. Feel free to contact the maintainers if that's a concern.

abhi-bhatra avatar Mar 09 '24 17:03 abhi-bhatra

@abhi-bhatra , could you please add a link into CHANGELOG.md? See instructions inside the file on how to do that.

nmanovic avatar Mar 12 '24 09:03 nmanovic

@abhi-bhatra thanks for the contribution I have one question: Are you sure that the current versions of Redis, Postgresql and Clickhouse charts will be compatible with all future versions of common chart v2.x.x?

azhavoro avatar Mar 12 '24 09:03 azhavoro

Hi @azhavoro Thanks for looking at the contribution. I am sure about the version compatibility. I have checked version of common chart in all these 3 charts, and used the static version which will not create an ambiguity of choosing a version by other charts.

dependencies:
- name: common
  repository: https://charts.bitnami.com/bitnami
  tags:
  - bitnami-common
  version: 2.x.x

But, still I will run few more test by changing versions of common chart and will see that if it is compatible with Redis, Postgresql and Clickhouse charts.

abhi-bhatra avatar Mar 12 '24 09:03 abhi-bhatra

Codecov Report

Merging #7581 (6574bd5) into develop (f513aa1) will decrease coverage by 0.12%. Report is 39 commits behind head on develop. The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7581      +/-   ##
===========================================
- Coverage    83.47%   83.35%   -0.12%     
===========================================
  Files          373      373              
  Lines        39739    39722      -17     
  Branches      3741     3747       +6     
===========================================
- Hits         33171    33112      -59     
- Misses        6568     6610      +42     
Components Coverage Δ
cvat-ui 79.20% <ø> (-0.09%) :arrow_down:
cvat-server 87.21% <100.00%> (-0.13%) :arrow_down:

codecov[bot] avatar Mar 22 '24 10:03 codecov[bot]

Wouldn't it be better to pin the common chart version (or at least a minor version) in case of possible backwards compatibility problems in the future?

Also, probably need to add a simple test to sure cvat can be deployed without analytics, which would certainly help to quickly detect possible problems in the future.

azhavoro avatar Mar 22 '24 12:03 azhavoro

@abhi-bhatra , will you be able to complete the PR?

nmanovic avatar Mar 26 '24 09:03 nmanovic

HI @nmanovic I am working on it, and will complete it by today EOD

abhi-bhatra avatar Mar 26 '24 10:03 abhi-bhatra

@azhavoro I have pinned the common chart version, but I would like you to point me where to add test for helm charts ?

Should I create a bash script ?

abhi-bhatra avatar Mar 26 '24 20:03 abhi-bhatra

@azhavoro I have pinned the common chart version, but I would like you to point me where to add test for helm charts ?

Should I create a bash script ?

I think it can be separate job in this workflow https://github.com/opencv/cvat/blob/develop/.github/workflows/helm.yml

azhavoro avatar Mar 28 '24 12:03 azhavoro

Thanks @azhavoro I am adding test case in this workflow. Thanks for guiding me !

abhi-bhatra avatar Mar 28 '24 18:03 abhi-bhatra

@azhavoro I have added a simple test case in the workflow, Can you review it ?

abhi-bhatra avatar Mar 30 '24 14:03 abhi-bhatra

@abhi-bhatra Hello, the test example is not working, could you please fix it?

azhavoro avatar Apr 09 '24 11:04 azhavoro

I can't merge this PR due to failed tests

azhavoro avatar May 24 '24 10:05 azhavoro