F/api domain config #4
Conversation
Release v1.0
Added codeowners
use docker check fix
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ikarakayali, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's configuration capabilities by introducing a configurable API domain, moving away from a hardcoded value. This change improves the system's adaptability across different environments. Additionally, it refines the handling of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Reviewer's GuideAdds configurable API domain support and normalizes USE_DOCKER handling across commands, wiring the domain through workflow API calls and extending default config. Sequence diagram for workflow sync with configurable API domainsequenceDiagram
actor Developer
participant CLI as syncCommand
participant Config as config
participant Workflow as processWorkflow
participant API as postWorkflow
participant Activator as activateWorkflow
Developer->>CLI: run sync
CLI->>Config: get API_BASE_URL, API_VERSION, API_DOMAIN
Config-->>CLI: apiConfig { baseUrl, version, domain }
CLI->>Workflow: processWorkflow(jsonPath, dbConfig, apiConfig)
Workflow->>API: postWorkflow(baseUrl, version, domain, flow, data)
API-->>Workflow: postResult { id }
Workflow->>Activator: activateWorkflow(baseUrl, version, domain, flow, instanceId, workflowVersion)
Activator-->>Workflow: activationResult
Workflow-->>CLI: { key, version, instanceId }
CLI-->>Developer: sync completed
Class diagram for config handling and workflow API callsclassDiagram
class ConfigStore {
+AUTO_DISCOVER : boolean
+API_BASE_URL : string
+API_VERSION : string
+API_DOMAIN : string
+DB_HOST : string
+DB_PORT : number
+DB_NAME : string
+DB_USER : string
+DB_PASSWORD : string
+USE_DOCKER : boolean|string
+DOCKER_POSTGRES_CONTAINER : string
+get(key)
}
class DbConfig {
+host : string
+port : number
+database : string
+user : string
+password : string
+useDocker : boolean
+dockerContainer : string
}
class ApiConfig {
+baseUrl : string
+version : string
+domain : string
}
class SyncCommand {
+syncCommand()
}
class ResetCommand {
+resetCommand(options)
}
class UpdateCommand {
+updateCommand(options)
}
class CheckCommand {
+checkCommand()
}
class WorkflowLib {
+processWorkflow(jsonPath, dbConfig, apiConfig)
}
class ApiLib {
+testApiConnection(baseUrl)
+postWorkflow(baseUrl, version, domain, flow, data)
+activateWorkflow(baseUrl, version, domain, flow, instanceId, workflowVersion)
}
ConfigStore <.. SyncCommand : uses
ConfigStore <.. ResetCommand : uses
ConfigStore <.. UpdateCommand : uses
ConfigStore <.. CheckCommand : uses
SyncCommand --> DbConfig : builds
SyncCommand --> ApiConfig : builds
ResetCommand --> DbConfig : builds
ResetCommand --> ApiConfig : builds
UpdateCommand --> DbConfig : builds
UpdateCommand --> ApiConfig : builds
CheckCommand --> DbConfig : builds
SyncCommand --> WorkflowLib : calls processWorkflow
ResetCommand --> WorkflowLib : calls processWorkflow
UpdateCommand --> WorkflowLib : calls processWorkflow
WorkflowLib --> ApiLib : calls postWorkflow
WorkflowLib --> ApiLib : calls activateWorkflow
DbConfig --> ConfigStore : derives useDocker from USE_DOCKER
ApiConfig --> ConfigStore : reads API_BASE_URL, API_VERSION, API_DOMAIN
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
USE_DOCKERnormalization logic (useDockerValue === true || useDockerValue === 'true') is duplicated across multiple commands; consider extracting a small helper or normalizing this once in the config layer to avoid repetition and keep the boolean parsing consistent. - Now that
API_DOMAINis part of the configuration and passed through multiple layers, it may be worth centralizing URL construction (e.g., a helper that builds the full workflow URL frombaseUrl,version, anddomain) to reduce the risk of future inconsistencies in API endpoint paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `USE_DOCKER` normalization logic (`useDockerValue === true || useDockerValue === 'true'`) is duplicated across multiple commands; consider extracting a small helper or normalizing this once in the config layer to avoid repetition and keep the boolean parsing consistent.
- Now that `API_DOMAIN` is part of the configuration and passed through multiple layers, it may be worth centralizing URL construction (e.g., a helper that builds the full workflow URL from `baseUrl`, `version`, and `domain`) to reduce the risk of future inconsistencies in API endpoint paths.
## Individual Comments
### Comment 1
<location> `src/commands/sync.js:32-39` </location>
<code_context>
// DB kontrolü
let dbSpinner = ora('Veritabanı kontrolü...').start();
try {
+ const useDockerValue = config.get('USE_DOCKER');
const isDbOk = await testDbConnection({
host: config.get('DB_HOST'),
port: config.get('DB_PORT'),
database: config.get('DB_NAME'),
user: config.get('DB_USER'),
password: config.get('DB_PASSWORD'),
- useDocker: config.get('USE_DOCKER'),
+ useDocker: useDockerValue === true || useDockerValue === 'true',
dockerContainer: config.get('DOCKER_POSTGRES_CONTAINER')
});
</code_context>
<issue_to_address>
**suggestion:** Normalize USE_DOCKER in a shared helper instead of repeating logic across commands.
This normalization (`value === true || value === 'true'`) now appears in `sync.js`, `reset.js`, `update.js`, and `check.js`. Consider moving it into a shared helper (e.g., in `config.js` or a `getUseDockerFlag()` function) so the interpretation of `USE_DOCKER` is defined in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const useDockerValue = config.get('USE_DOCKER'); | ||
| const dbConfig = { | ||
| host: config.get('DB_HOST'), | ||
| port: config.get('DB_PORT'), | ||
| database: config.get('DB_NAME'), | ||
| user: config.get('DB_USER'), | ||
| password: config.get('DB_PASSWORD'), | ||
| useDocker: config.get('USE_DOCKER'), | ||
| useDocker: useDockerValue === true || useDockerValue === 'true', |
There was a problem hiding this comment.
suggestion: Normalize USE_DOCKER in a shared helper instead of repeating logic across commands.
This normalization (value === true || value === 'true') now appears in sync.js, reset.js, update.js, and check.js. Consider moving it into a shared helper (e.g., in config.js or a getUseDockerFlag() function) so the interpretation of USE_DOCKER is defined in one place.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration for the API domain, which is a great improvement for flexibility. The implementation is solid, correctly propagating the new API_DOMAIN config value through the different layers of the application.
I've also noticed a change to handle both boolean and string values for the USE_DOCKER configuration. While this is a good defensive measure, the implementation is duplicated across four different files (check.js, reset.js, sync.js, update.js). I've left a comment with a suggestion to simplify the logic and centralize it to improve code maintainability.
| user: config.get('DB_USER'), | ||
| password: config.get('DB_PASSWORD'), | ||
| useDocker: config.get('USE_DOCKER'), | ||
| useDocker: useDockerValue === true || useDockerValue === 'true', |
There was a problem hiding this comment.
This check for a boolean or string 'true' can be simplified. By performing the conversion inline, you can also remove the temporary useDockerValue variable. This same pattern is repeated in reset.js, sync.js, and update.js. Consider refactoring this logic into a shared utility function to improve maintainability and avoid code duplication.
| useDocker: useDockerValue === true || useDockerValue === 'true', | |
| useDocker: String(config.get('USE_DOCKER')) === 'true', |
Summary by Sourcery
Add configurable API domain support and normalize Docker usage configuration across workflow commands.
Bug Fixes:
Enhancements: