Skip to content

refactor(config): rename ANALYTICS_ENABLED to IS_ANALYTICS_ENABLED #11652

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

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

Conversation

AMoreaux
Copy link
Contributor

Updated all occurrences of ANALYTICS_ENABLED to IS_ANALYTICS_ENABLED for better naming consistency. Adjusted environment variables, code references, and tests accordingly to reflect the change.

@AMoreaux AMoreaux requested a review from FelixMalfait April 18, 2025 14:11
@AMoreaux AMoreaux self-assigned this Apr 18, 2025
Copy link
Contributor

github-actions bot commented Apr 18, 2025

Warnings
⚠️

Changes were made to package.json, but not to yarn.lock - Perhaps you need to run yarn install?

⚠️ Changes were made to the config variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

Generated by 🚫 dangerJS against 60c76ee

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces changes to standardize environment variable naming while also making significant security and database configuration changes.

  • CRITICAL: Disabling TLS certificate validation in packages/twenty-front/codegen.cjs and codegen-metadata.cjs with NODE_TLS_REJECT_UNAUTHORIZED = '0' creates serious security vulnerabilities
  • Added allow_experimental_json_type and index_granularity=8192 settings to ClickHouse tables in packages/twenty-server/src/database/clickhouse/migrations/001-create-events-table.sql and 002-create-pageview-table.sql
  • Renamed ANALYTICS_ENABLED to IS_ANALYTICS_ENABLED across multiple files for consistent boolean naming convention
  • Potential issue in analytics.service.ts where AnalyticsException is returned instead of thrown in preventAnalyticsIfDisabled method

18 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Updated all occurrences of `ANALYTICS_ENABLED` to `IS_ANALYTICS_ENABLED` for better naming consistency. Adjusted environment variables, code references, and tests accordingly to reflect the change. line setting NODE_TLS_REJECT_UNAUTHORIZED to '0'. This improves security by no longer bypassing TLS certificate validation.
@AMoreaux AMoreaux force-pushed the fix/adapte-migration-to-clickhouse-cloud branch from 4db6829 to 8c76260 Compare April 18, 2025 14:19
Added a new script command to run ClickHouse migrations in production. This ensures proper database setup for ClickHouse during deployment.
Removed the command to enable the experimental JSON type as it is no longer required. This simplifies the migration script and aligns with updated database configuration practices.
Cleaned up unnecessary ESLint disable comments and outdated TODO notes across various components. This improves code readability and reduces clutter without affecting functionality.
@@ -29,7 +29,7 @@ export const GET_CLIENT_CONFIG = gql`
defaultSubdomain
frontDomain
debugMode
analyticsEnabled
isAnalyticsEnabled
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's rename the env variable only since this will create a breaking change during the next deploy :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants