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

Add Parquet-Exporter helm chart #195

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

Conversation

cklingspor
Copy link

Moved from opencost/opencost-parquet-exporter#10

Hey everyone,

I've created a first draft of a helm chart for the parquet-exporter. I do not have a setup where I can use AWS as of now, therefore the actual push to a S3 bucket is not yet tested. However, I was able to install the chart and run the cronjob in my minikube.

Used helm-docs to create a rough documentation around the chart
The chart itself is relatively straight forward I think
Let me know what you think!

@@ -0,0 +1,24 @@
apiVersion: v2
name: parquet-exporter
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we make this "opencost-parquet-exporter"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That will have a ripple effect in the templates, but I prefer the verbosity/specificity

@@ -0,0 +1,24 @@
apiVersion: v2
name: parquet-exporter
description: A Helm chart for Kubernetes
Copy link
Collaborator

Choose a reason for hiding this comment

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

"OpenCost Parquet Exporter"

@@ -0,0 +1,38 @@
# parquet-exporter
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make the title and the path charts/opencost-parquet-exporter to match the repository?


| Key | Type | Default | Description |
|-----|------|---------|-------------|
| activeDeadlineSeconds | int | `3600` | Keep job runnig (from start time) for [activeDeadlineSeconds] |
Copy link
Collaborator

Choose a reason for hiding this comment

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

"running"

charts/parquet-exporter/values.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
apiVersion: v2
name: parquet-exporter
Copy link
Collaborator

Choose a reason for hiding this comment

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

That will have a ripple effect in the templates, but I prefer the verbosity/specificity

Copy link
Contributor

Choose a reason for hiding this comment

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

add new line at the end

charts/parquet-exporter/values.yaml Outdated Show resolved Hide resolved
charts/parquet-exporter/templates/cronjob.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

add new line at the end

Copy link
Contributor

@asdfgugus asdfgugus left a comment

Choose a reason for hiding this comment

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

I did not test it on my cluster, but the code LGTM.

Copy link
Collaborator

@mattray mattray left a comment

Choose a reason for hiding this comment

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

I think the values need to move to opencost-parquet-exporter, but I didn't test.

Copy link

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jun 16, 2024
@mattray mattray removed the Stale label Jun 18, 2024
Copy link

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Aug 18, 2024
@lmello lmello added enhancement New feature or request and removed Stale labels Aug 21, 2024
@lmello
Copy link
Member

lmello commented Aug 21, 2024

I will review this PR after the change to the parquet export repo is merged.
we cannot merge this PR before this is done.

Copy link
Member

@lmello lmello left a comment

Choose a reason for hiding this comment

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

Please fix the trailling whitespaces:
Error: 1:56 [trailing-spaces] trailing spaces
Error: 8:22 [trailing-spaces] trailing spaces
Error: 12:68 [trailing-spaces] trailing spaces
Error: 27:12 [trailing-spaces] trailing spaces
Error: 59:209 [trailing-spaces] trailing spaces
Error: 62:117 [trailing-spaces] trailing spaces

Copy link
Contributor

@brito-rafa brito-rafa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Oct 27, 2024
@github-actions github-actions bot removed the Stale label Nov 1, 2024
Copy link

github-actions bot commented Jan 2, 2025

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jan 2, 2025
@cklingspor cklingspor requested review from lmello and mattray January 2, 2025 10:01
@github-actions github-actions bot removed the Stale label Jan 3, 2025
@cklingspor
Copy link
Author

@lmello Sorry, for missing this. Done

@lmello
Copy link
Member

lmello commented Jan 15, 2025

Thanks @cklingspor, I will merge this.

@lmello
Copy link
Member

lmello commented Jan 15, 2025

@cklingspor do you want to be the maintainer of the helm chart ? if yes you can add yourself to the maintainers on it to fix the CI error. if not, you can add my name on it.

@cklingspor
Copy link
Author

@cklingspor do you want to be the maintainer of the helm chart ? if yes you can add yourself to the maintainers on it to fix the CI error. if not, you can add my name on it.

Sure! I've added my name. Will add way to contact later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants