Skip to content

Conversation

@alespour
Copy link
Contributor

@alespour alespour commented Dec 10, 2025

Fixed most important (all critical and medium, and some low prio) issues per @jansimonb's review of PR #752:

# Issue Severity Status
1 Commercial License Configuration Bug Critical FIXED
9 Wrong Environment Variable Prefix for Traces Critical FIXED
3 Object Storage Issues Medium FIXED
7 Missing Component Enabled Checks in Templates Medium FIXED
2 Dead Configuration Options Low FIXED (SUBSET)
4 Component Configuration Issues Low FIXED
5 Dead Code in _helpers.tpl Low FIXED
8 Confusing YAML Formatting in Commented Sections Low FIXED
10 Documentation Issues Low FIXED (SUBSET)

@alespour alespour requested a review from Copilot December 10, 2025 13:26
@alespour alespour marked this pull request as ready for review December 10, 2025 13:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request addresses critical and medium severity issues identified in a code review of the InfluxDB3 Enterprise Helm chart. The changes fix important bugs in license configuration, trace environment variables, object storage handling, and component-enabled checks across templates.

  • Corrects the license file configuration to use a file path mount instead of embedding content in environment variables
  • Fixes environment variable prefixes for traces (removes incorrect INFLUXDB3_ prefix)
  • Adds support for AWS credentials file as an alternative to access key/secret key authentication
  • Adds conditional checks to prevent creating resources (Ingress, NetworkPolicy) for disabled components
  • Removes dead code from helper templates and fixes YAML formatting issues

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
charts/influxdb3-enterprise/Chart.yaml Bumps chart version from 0.1.0 to 0.1.1
charts/influxdb3-enterprise/values.yaml Updates object storage comments, removes dead persistence.enabled fields, fixes PDB maxUnavailable, improves documentation
charts/influxdb3-enterprise/templates/secret-object-storage.yaml Adds AWS credentials file secret creation and updates credential detection logic
charts/influxdb3-enterprise/templates/processor-statefulset.yaml Adds initContainer for plugin installation, removes conditional wrapping of volume mounts and volumeClaimTemplates
charts/influxdb3-enterprise/templates/networkpolicy.yaml Adds component-enabled checks to all NetworkPolicy resources
charts/influxdb3-enterprise/templates/ingress-write.yaml Adds ingester.enabled check to prevent creating Ingress when component is disabled
charts/influxdb3-enterprise/templates/ingress-query.yaml Adds querier.enabled check to prevent creating Ingress when component is disabled
charts/influxdb3-enterprise/templates/configmap.yaml Fixes trace environment variable prefixes, adds object storage validation, adds AWS credentials file support
charts/influxdb3-enterprise/templates/compactor-statefulset.yaml Removes empty conditional block
charts/influxdb3-enterprise/templates/_helpers.tpl Removes unused helper functions, adds object storage type validation, fixes license file mounting with subPath
charts/influxdb3-enterprise/templates/NOTES.txt Simplifies license configuration warning logic
charts/influxdb3-enterprise/examples/values-dev.yaml Removes dead enabled fields from file storage configuration
charts/influxdb3-enterprise/README.md Updates license.file parameter documentation to clarify --set-file usage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@alespour alespour requested a review from srebhan December 10, 2025 13:55
@alespour alespour requested review from jansimonb and removed request for srebhan December 10, 2025 13:55
jansimonb
jansimonb previously approved these changes Dec 11, 2025
Copy link
Contributor

@jansimonb jansimonb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍 I had just minor comments.

Co-authored-by: Jan Simon <63776254+jansimonb@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alespour alespour requested a review from jansimonb December 11, 2025 17:06
jansimonb
jansimonb previously approved these changes Dec 11, 2025
Copy link
Contributor

@jansimonb jansimonb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Co-authored-by: Jan Simon <63776254+jansimonb@users.noreply.github.com>
@alespour
Copy link
Contributor Author

So when I commit reviewer's suggestion it dismisses the review. wth...
@jansimonb I am sorry please approve for the last time

@alespour alespour merged commit 0451b3a into master Dec 11, 2025
2 checks passed
@alespour alespour deleted the fix/critical-issues-js-review-1 branch December 11, 2025 20:12
@jansimonb
Copy link
Contributor

no problem, this is set in project's settings :)

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.

3 participants