feat: allow to set sourceRegistry by CNPMCORE_CONFIG_SOURCE_REGISTRY #753
Conversation
WalkthroughThis pull request centralizes and standardizes environment variable retrieval by introducing a new type and overloading the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Config/Database Module
participant E as EnvUtil.env()
participant OS as process.env
C->>E: Request env variable with key, type, defaultValue
E->>OS: Retrieve value using key
alt Value exists and is valid
E-->>C: Return parsed (string/boolean/number) value
else Value missing or invalid
E-->>C: Return default value or throw TypeError
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
将环境变量解析做了一下封装 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
config/config.default.ts (1)
209-220: Add validation for Elasticsearch configuration.Similar to OSS and S3 configurations, add assertions for required Elasticsearch environment variables.
if (config.cnpmcore.enableElasticsearch) { + const node = env('CNPMCORE_CONFIG_ES_CLIENT_NODE', 'string', ''); + const username = env('CNPMCORE_CONFIG_ES_CLIENT_AUTH_USERNAME', 'string', ''); + const password = env('CNPMCORE_CONFIG_ES_CLIENT_AUTH_PASSWORD', 'string', ''); + assert(node, 'require env CNPMCORE_CONFIG_ES_CLIENT_NODE'); + assert(username, 'require env CNPMCORE_CONFIG_ES_CLIENT_AUTH_USERNAME'); + assert(password, 'require env CNPMCORE_CONFIG_ES_CLIENT_AUTH_PASSWORD'); config.elasticsearch = { client: { - node: env('CNPMCORE_CONFIG_ES_CLIENT_NODE', 'string', ''), + node, auth: { - username: env('CNPMCORE_CONFIG_ES_CLIENT_AUTH_USERNAME', 'string', ''), - password: env('CNPMCORE_CONFIG_ES_CLIENT_AUTH_PASSWORD', 'string', ''), + username, + password, }, }, }; }
🧹 Nitpick comments (5)
app/common/EnvUtil.ts (2)
16-26: Consider enhancing boolean parsing flexibility.The current boolean parsing is strict and might be too restrictive. Consider supporting additional common boolean representations.
- if (value === 'true' || value === '1') { + const trueValues = ['true', '1', 'yes', 'y']; + const falseValues = ['false', '0', 'no', 'n']; + if (trueValues.includes(value.toLowerCase())) { booleanValue = true; - } else if (value === 'false' || value === '0') { + } else if (falseValues.includes(value.toLowerCase())) { booleanValue = false; } else { - throw new TypeError(`Invalid boolean value: ${value} on process.env.${key}`); + throw new TypeError(`Invalid boolean value: ${value} for ${key}. Expected one of: ${[...trueValues, ...falseValues].join(', ')}`); }
28-34: Enhance number validation.Consider adding range validation for numeric values to prevent potential issues.
if (valueType === 'number') { const numberValue = Number(value); if (isNaN(numberValue)) { - throw new TypeError(`Invalid number value: ${value} on process.env.${key}`); + throw new TypeError(`Invalid number value: ${value} for ${key}. Value must be a valid number.`); } + if (!Number.isFinite(numberValue)) { + throw new TypeError(`Invalid number value: ${value} for ${key}. Value must be finite.`); + } return numberValue; }config/database.ts (1)
32-32: Track SQLite implementation.The TODO comment for SQLite implementation should be tracked in an issue.
Would you like me to create an issue to track the SQLite implementation?
config/config.default.ts (2)
135-138: Improve assertion messages for OSS configuration.The assertion messages should match the environment variable names they're checking.
- assert(ossConfig.cdnBaseUrl, 'require env CNPMCORE_NFS_OSS_BUCKET'); - assert(ossConfig.endpoint, 'require env CNPMCORE_NFS_OSS_ENDPOINT'); - assert(ossConfig.accessKeyId, 'require env CNPMCORE_NFS_OSS_ID'); - assert(ossConfig.accessKeySecret, 'require env CNPMCORE_NFS_OSS_SECRET'); + assert(ossConfig.cdnBaseUrl, 'require env CNPMCORE_NFS_OSS_CDN'); + assert(ossConfig.endpoint, 'require env CNPMCORE_NFS_OSS_ENDPOINT'); + assert(ossConfig.accessKeyId, 'require env CNPMCORE_NFS_OSS_ID'); + assert(ossConfig.accessKeySecret, 'require env CNPMCORE_NFS_OSS_SECRET');
156-159: Consider using dynamic import for S3 client.The TODO comment suggests using import for ESM support. This can be implemented now.
- // TODO(@fengmk2): should change to use import to support esm - // eslint-disable-next-line @typescript-eslint/no-var-requires - const S3Client = require('s3-cnpmcore'); - config.nfs.client = new S3Client(s3Config); + const { default: S3Client } = await import('s3-cnpmcore'); + config.nfs.client = new S3Client(s3Config);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/common/EnvUtil.ts(1 hunks)config/config.default.ts(9 hunks)config/database.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
config/database.ts
[error] 1-1: Strings must use singlequote.
(quotes)
🪛 GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Check: test-postgresql-fs-nfs (18, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Check: test-mysql57-fs-nfs (18.20.0, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Check: test-postgresql-fs-nfs (18.20.0, ubuntu-latest)
config/database.ts
[failure] 1-1:
Strings must use singlequote
🪛 GitHub Actions: Node.js CI
config/database.ts
[error] 1-1: Strings must use singlequote quotes.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
app/common/EnvUtil.ts (1)
1-1: LGTM! Type definition is clear and concise.The
ValueTypetype accurately represents the supported value types.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #753 +/- ##
==========================================
- Coverage 96.47% 96.35% -0.12%
==========================================
Files 191 192 +1
Lines 19232 19274 +42
Branches 2508 2513 +5
==========================================
+ Hits 18554 18572 +18
- Misses 678 702 +24 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/database.ts (1)
32-32: Track SQLite implementation.The TODO comment indicates pending SQLite implementation.
Would you like me to help implement the SQLite configuration or create an issue to track this task?
prepare-database-mysql.sh (1)
3-3: Enable shell error handling and debugging.Uncomment the
set -exline to enable immediate exit on errors and command tracing, which will help with debugging database preparation issues.Apply this diff:
-# set -ex +set -exprepare-database-postgresql.sh (1)
6-9: Update PostgreSQL Default Connection Values.
The script now provides sensible defaults for connecting to PostgreSQL:
• db_host defaults to 127.0.0.1,
• db_username and db_password default to "postgres".
Note: The inline comment on line 9 (“# default to empty password”) should be updated to reflect the new default value ("postgres").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
-
.env.example(1 hunks) -
DEVELOPER.md(3 hunks) -
config/config.unittest.ts(1 hunks) -
config/database.ts(1 hunks) -
docker-compose-postgres.yml(1 hunks) -
docker-compose.yml(2 hunks) -
prepare-database-mysql.sh(1 hunks) -
prepare-database-postgresql.sh(1 hunks) -
test/TestUtil.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (18, ubuntu-latest)
- GitHub Check: Analyze (typescript)
- GitHub Check: test-mysql57-fs-nfs (18.20.0, ubuntu-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: test-postgresql-fs-nfs (18.20.0, ubuntu-latest)
🔇 Additional comments (24)
config/config.unittest.ts (1)
13-13: Verify the impact of using logical OR operator.The change from nullish coalescing (
??) to logical OR (||) means the default value'cnpmcore_unittest'will be used for any falsydatabase.namevalue (empty string, 0, false), not just null/undefined. Ensure this aligns with the expected test behavior.config/database.ts (3)
1-1: Fix string quote style.Use single quotes consistently throughout the file to comply with the project's style guide.
9-14: Consider more secure default values.Empty strings and zero values as defaults for database credentials could lead to unintended connections.
29-30:⚠️ Potential issueRemove hard-coded PostgreSQL credentials.
Hard-coded credentials pose a security risk and should be replaced with environment variables.
Apply this diff to remove hard-coded credentials:
- dbUser = dbUser || env('CNPMCORE_POSTGRES_USER', 'string', '') || env('POSTGRES_USER', 'string', '') || 'postgres'; - dbPassword = dbPassword || env('CNPMCORE_POSTGRES_PASSWORD', 'string', '') || env('POSTGRES_PASSWORD', 'string', '') || 'postgres'; + dbUser = dbUser || env('CNPMCORE_POSTGRES_USER', 'string', '') || env('POSTGRES_USER', 'string', ''); + dbPassword = dbPassword || env('CNPMCORE_POSTGRES_PASSWORD', 'string', '') || env('POSTGRES_PASSWORD', 'string', '');Likely invalid or redundant comment.
test/TestUtil.ts (1)
52-52: Verify consistency with test configuration.The change from nullish coalescing (
??) to logical OR (||) matches the change inconfig/config.unittest.ts. Ensure this behavior is consistent with test requirements.docker-compose.yml (8)
1-1: Clear Environment Name Declaration.
Setting the service name to "cnpmcore_dev_services_mysql" clearly identifies the intended development environment.
5-6: Centralize Redis Configuration with env_file.
Adding the env_file directive for the redis service ensures that environment variables are maintained in one place (the .env file), which enhances consistency and simplifies configuration management.
15-15: Consistent Network Assignment for Redis.
The update to assign the redis service to the "cnpm-mysql" network maintains connectivity consistency across services.
17-20: MySQL Service: Use of env_file and Updated Image.
Including the env_file for the MySQL service and updating the image to "mysql:9" help centralize environment configuration and align with required database version preferences. Please verify that version 9 is fully compatible with your application’s needs.
23-27: MySQL Service: Dynamic Environment Variables.
Defining MYSQL_ROOT_PASSWORD and MYSQL_DATABASE with default value expressions (e.g., ${MYSQL_ROOT_PASSWORD:-} and ${MYSQL_DATABASE:-cnpmcore}) improves portability and makes it easier to override these settings as needed.
31-32: Network Update for MySQL Service.
Switching the network to "cnpm-mysql" ensures that the MySQL container connects to the correct network for this configuration.
36-46: phpMyAdmin Service: Environment and Comments Verification.
Adding an env_file along with dynamic environment variable references for phpMyAdmin (and commenting out MYSQL_USER/PASSWORD) supports consistency with the other services. The commented variables are acceptable if they’re not required in the current configuration.
58-61: Custom Network Definition Updated.
The networks section now defines "cnpm-mysql" with a clear name and bridge driver, providing a dedicated network for database-related services.docker-compose-postgres.yml (6)
1-1: Distinct Service Name for PostgreSQL Development.
The name "cnpmcore_dev_services_postgres" clearly differentiates this setup from the MySQL-based environment, aiding in environment management.
4-6: Redis Service Configuration for PostgreSQL Environment.
The redis service is configured with an env_file and appropriate mappings, aligning with the overall centralized configuration approach.
17-20: Postgres Service: Centralized Environment and Image Selection.
Using an env_file along with explicit environment variable settings for POSTGRES_USER and POSTGRES_PASSWORD ensures that the container inherits the correct credentials. The selection of the "postgres:17" image should be verified for compatibility (as version 17 is less common and may be a recent release).
22-24: Dynamic Environment Variables for Postgres.
The configuration defaults for POSTGRES_USER and POSTGRES_PASSWORD are set with fallback values ("postgres"), which is in line with best practices for ensuring a secure and predictable environment.
32-43: pgAdmin Service: Comprehensive Environment Setup.
The pgAdmin service correctly sources its environment settings (including default credentials and configuration parameters) from the .env file. This promotes consistency and facilitates easier configuration updates.
53-62: Volumes and Network Definition for PostgreSQL Setup.
The volumes and the custom network ("cnpm-postgres") sections are clearly defined, which supports container isolation and persistence management.DEVELOPER.md (5)
7-11: Documentation: Generating Local Environment Configuration.
The added instructions for generating a local .env configuration file (by copying .env.example) are clear and user friendly. This aids developers in quickly setting up their local development environment.
15-23: MySQL Environment Setup Instructions.
The documentation now provides step‐by‐step commands for starting (and stopping) MySQL + Redis services using docker-compose.yml. The code block is clear and instructive.
25-33: PostgreSQL Environment Setup Instructions.
The added section for setting up PostgreSQL + Redis using docker-compose-postgres.yml is well structured and mirrors the MySQL setup instructions. This consistency supports ease of use for developers preferring either database option.
57-63: Login and Package Publishing Procedures.
The newly added instructions for logging in as an administrator (using npm login) and testing package publishing directly to the local registry are very helpful. They support quick verification of environment configurations and developer workflows.
74-89: Expanded Developer Guidelines and Operational Instructions.
The additional sections covering how to run unit tests for both MySQL and PostgreSQL, as well as the extended documentation on project structure and service guidelines, enhance developer clarity. The comprehensive nature of these instructions minimizes onboarding friction and supports efficient development practices.
[skip ci] ## [3.74.0](v3.73.1...v3.74.0) (2025-02-09) ### Features * allow to set sourceRegistry by CNPMCORE_CONFIG_SOURCE_REGISTRY ([#753](#753)) ([9f4f1f1](9f4f1f1))
Improve the local development process based on docker-compose
Summary by CodeRabbit