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

[data-dashboard-backend] Update charts for data dashboard backend service #196

Merged
merged 3 commits into from
May 29, 2024

Conversation

pvannierop
Copy link
Collaborator

@pvannierop pvannierop commented May 28, 2024

Description of the change

Data dashboard backend is a new service that exposes data from the observations table in timescaledb database (provisioned by KSQLDB service) to frontend/mobile applications. This PR will improve the helm chart for this service.

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [<name_of_the_chart>])

@pvannierop pvannierop self-assigned this May 28, 2024
Copy link

github-actions bot commented May 28, 2024

Great PR! Please pay attention to the following items before merging:

Files matching charts/*/values.yaml:

  • Is the PR adding a new container? Please reviewer, add it to the models (internal process)
  • Is the PR adding a new parameter? Please, ensure it’s documented in the README.md

This is an automatically generated QA checklist based on modified files.

@pvannierop pvannierop force-pushed the data-dashboard-backend branch 3 times, most recently from 92dd4be to 71ca874 Compare May 28, 2024 14:40
# -- image pull policy
pullPolicy: Always
# Overrides the image tag whose default is the chart appVersion.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comments should start with a -- in order to be added into README.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not know. Fixed!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't seem to be fixed

port: http
httpHeaders:
- name: Accept
value: application/json
readinessProbe:
httpGet:
path: /api/health
path: {{ .Values.path }}/health
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really related to this PR but some of our components are pointed to a sub domain and some other to prefixes in the main domain and it's not clear for me when we use which one. I think we should have an agreement on it. I'll make an issue for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, the grizzly application has this as a configurable option. The sub-path can be modified in the helmfile. I think that for this reason the helm chart must allow configuration of this path, in order to allow for outside changes of subpath?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that is true. It's good to have this configurable.

@keyvaann
Copy link
Collaborator

Thanks @pvannierop, please also add an entry for this component in the radar-home chart.

@pvannierop
Copy link
Collaborator Author

@keyvaann

Thanks @pvannierop, please also add an entry for this component in the radar-home chart.

This is only a backend service. I think the home page only links to frontend applications, right? If so, we do not need to add it.

@pvannierop pvannierop requested a review from keyvaann May 29, 2024 06:58
@keyvaann
Copy link
Collaborator

@keyvaann

Thanks @pvannierop, please also add an entry for this component in the radar-home chart.

This is only a backend service. I think the home page only links to frontend applications, right? If so, we do not need to add it.

That's right, I didn't notice that this was a backend only app.

@pvannierop pvannierop force-pushed the data-dashboard-backend branch 3 times, most recently from f079969 to dbd77ea Compare May 29, 2024 07:36
Copy link
Collaborator

@keyvaann keyvaann left a comment

Choose a reason for hiding this comment

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

LGTM

@pvannierop pvannierop force-pushed the data-dashboard-backend branch 2 times, most recently from 6bcd429 to b27bf22 Compare May 29, 2024 08:07
@pvannierop pvannierop force-pushed the data-dashboard-backend branch from 8a1b44a to 87e358d Compare May 29, 2024 08:13
@pvannierop pvannierop requested a review from keyvaann May 29, 2024 09:04
@keyvaann keyvaann merged commit 858d52c into main May 29, 2024
4 checks passed
@keyvaann keyvaann deleted the data-dashboard-backend branch May 29, 2024 09:18
@pvannierop pvannierop changed the title [data-dashboard-backend] Add charts for data dashboard backend service [data-dashboard-backend] Update charts for data dashboard backend service May 29, 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