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

adding support to custom pricing configuration in helm chart #109

Conversation

HoussemCharf
Copy link
Contributor

Closes #108

@HoussemCharf HoussemCharf changed the title #108: adding support to custom pricing configuration in helm chart adding support to custom pricing configuration in helm chart Aug 28, 2023
@mattray
Copy link
Collaborator

mattray commented Aug 31, 2023

This looks like it duplicates the efforts of #107. Could you check that patch and see if there's anything in it this one needs? I do like this patch's use use of opencost.onpremisemode.enabled, so I'm leaning more towards it after it gets cleaned up.

@tamalsaha
Copy link

tamalsaha commented Aug 31, 2023

opencost.onpremisemode.enabled is semantically wrong for what I was doing in #107 .

The metricsConfigs.disabledMetrics is necessary to avoid duplicate KMS v2 metrics. But the configmap mounted can be used to mount cluster-info.json and metrics.json.

@HoussemCharf
Copy link
Contributor Author

@mattray From what I saw in his PR it tackles different issues with metrics configuration related to state referenced in here and here correct me if I am wrong @tamalsaha, but not custom pricing configuration for on-premise setup. But I'd be happy to include anything that you think relevant in his PR in my changes

@tamalsaha
Copy link

@HoussemCharf, yes. My pr tackles unrelated issues. Those are not related to onpremises deployment.

Copy link
Collaborator

@toscott toscott left a comment

Choose a reason for hiding this comment

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

I love the idea of simplifying the barrier for using custom pricing. Thank you for the suggestion and taking a stab at it!

I've made a few comments. Once addressed we can get this merged.

charts/opencost/values.yaml Outdated Show resolved Hide resolved
charts/opencost/values.yaml Outdated Show resolved Hide resolved
charts/opencost/values.yaml Outdated Show resolved Hide resolved
charts/opencost/templates/configmap.yaml Outdated Show resolved Hide resolved
@HoussemCharf
Copy link
Contributor Author

@toscott Thanks for your review I've addressed the issues mentioned.

@toscott
Copy link
Collaborator

toscott commented Sep 15, 2023

Awesome!
I plan to review, retest and get merged tonight

charts/opencost/templates/configmap.yaml Outdated Show resolved Hide resolved
charts/opencost/templates/configmap.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@toscott toscott left a comment

Choose a reason for hiding this comment

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

The last round of changes appears to be working well! Thanks for the contribution @HoussemCharf. Waiting on the CI flows to finish and I'll get this merged.

@toscott toscott merged commit c5f41f0 into opencost:main Sep 24, 2023
1 check passed
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.

Custom pricing configmap
4 participants