fix(helm): add cloudwatch region and logGroup fields to flyte-binary chart#7235
Open
nuthalapativarun wants to merge 2 commits intoflyteorg:masterfrom
Open
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
…chart The flyte-binary chart only supported cloudwatch-template-uri for CloudWatch log configuration, requiring users to embed the region and log group directly in a URL template string. When users deployed the eks example files without replacing the <AWS_REGION> and <EKS_CLUSTER_NAME> placeholders, these literal strings appeared URL-encoded in the Flyte UI CloudWatch links. Add dedicated cloudwatch.region and cloudwatch.logGroup fields to the chart, mirroring the configuration surface already available in flyte-core. When these fields are set, the chart emits cloudwatch-region and cloudwatch-log-group config keys. The existing templateUri field is preserved for full backward compatibility and takes precedence when set. Update eks-production.yaml and eks-starter.yaml to use the new fields. Closes flyteorg#7065 Signed-off-by: Varun Nuthalapati <nuthalapativarun@gmail.com>
nuthalapativarun
force-pushed
the
fix/flyte-binary-cloudwatch-config-7065
branch
from
71d6fc7 to
5a2ed9e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7235 +/- ##
==========================================
- Coverage 56.95% 56.92% -0.03%
==========================================
Files 931 931
Lines 58234 58234
==========================================
- Hits 33166 33150 -16
- Misses 22017 22032 +15
- Partials 3051 3052 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracking issue
Closes #7065
Why are the changes needed?
The
flyte-binarychart only exposedtemplateUrifor CloudWatch log configuration, requiring users to embed the AWS region and EKS cluster name directly in a URL template string. The eks example files shipped with literal<AWS_REGION>and<EKS_CLUSTER_NAME>placeholder strings. When users deployed without replacing these, the placeholders appeared URL-encoded (%3CAWS_REGION%3E) in the Flyte UI CloudWatch log links.The
flyte-corechart supports dedicatedcloudwatch-regionandcloudwatch-log-groupfields, which the backend uses to construct the CloudWatch URL internally. Theflyte-binarychart lacked equivalent fields.What changes were proposed in this pull request?
charts/flyte-binary/values.yaml: Addedcloudwatch.regionandcloudwatch.logGroupfields underconfiguration.logging.plugins.cloudwatch.charts/flyte-binary/templates/_helpers.tpl: Updated theflyte-binary.configuration.logging.pluginshelper to emitcloudwatch-regionandcloudwatch-log-groupwhen the new fields are set. The existingtemplateUrifield takes precedence when set, preserving full backward compatibility.charts/flyte-binary/eks-production.yamlandeks-starter.yaml: Replaced thetemplateUriwith the placeholder strings with the newregionandlogGroupfields, which users replace with their actual values.How was this patch tested?
Verified with
helm templatethat:cloudwatch.regionandcloudwatch.logGroupproducescloudwatch-regionandcloudwatch-log-groupconfig keys in the rendered configmap.cloudwatch.templateUristill producescloudwatch-template-urias before (backward-compatible path).Labels
fixed
I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.