Skip to content
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

[PLA-2013] Fixes bind port #42

Merged
merged 4 commits into from
Sep 24, 2024
Merged

[PLA-2013] Fixes bind port #42

merged 4 commits into from
Sep 24, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Sep 24, 2024

PR Type

enhancement, configuration changes


Description

  • Removed the defaultBindPort variable from bin/server.dart and set the bind port directly in the spawnServer function.
  • Updated the logger to reflect the new version 2.1.1.
  • Removed DEFAULT_BIND_PORT from the .env.example file.
  • Updated the pubspec.yaml file to bump the version from 2.1.0 to 2.1.1.

Changes walkthrough 📝

Relevant files
Enhancement
server.dart
Remove default bind port variable and update version         

bin/server.dart

  • Removed the defaultBindPort variable.
  • Updated the logger version information.
  • Set the bind port directly in the spawnServer function.
  • +2/-6     
    Configuration changes
    .env.example
    Remove DEFAULT_BIND_PORT from example environment file     

    .env.example

    • Removed the DEFAULT_BIND_PORT entry.
    +0/-1     
    pubspec.yaml
    Bump version to 2.1.1                                                                       

    pubspec.yaml

    • Updated the version from 2.1.0 to 2.1.1.
    +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Hardcoded Value
    The bind port is hardcoded to 8090 in spawnServer function. Consider making it configurable via environment variables or configuration files to enhance flexibility.

    Copy link

    github-actions bot commented Sep 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Replace hardcoded port number with a configurable environment variable

    Replace the hardcoded value of defaultBindPort with a configurable environment
    variable to maintain flexibility and consistency with other settings.

    bin/server.dart [47]

    -defaultBindPort: 8090,
    +defaultBindPort: int.tryParse(env.getOrElse('DEFAULT_BIND_PORT', () => '8090')) ?? 8090,
     
    Suggestion importance[1-10]: 9

    Why: This suggestion improves maintainability by allowing the port number to be configured via an environment variable, aligning with the existing pattern for other settings.

    9
    Dynamically log the application version from an environment variable

    Update the version log message to dynamically fetch the version from a single source
    of truth to avoid manual updates in multiple places.

    bin/server.dart [23]

    -logger.info('Starting Platform Decoder v2.1.1');
    +logger.info('Starting Platform Decoder v${env['VERSION']}');
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves maintainability by reducing the need for manual updates of the version number in multiple places, though it requires the version to be set in the environment.

    7
    Error handling
    Add error handling for environment variable parsing

    Consider adding error handling or logging for the parsing operations to ensure that
    any issues with environment variables are clearly reported.

    bin/server.dart [16-21]

    -numberOfIsolates =
    -    int.tryParse(env.getOrElse('NUMBER_OF_ISOLATES', () => '8')) ?? 8;
    -int specPerIsolate =
    -    int.tryParse(env.getOrElse('SPEC_PER_ISOLATE', () => '4')) ?? 4;
    -int specExpireDuration =
    -    int.tryParse(env.getOrElse('SPEC_EXPIRE_DURATION', () => '300')) ?? 300;
    +try {
    +  numberOfIsolates =
    +      int.parse(env.getOrElse('NUMBER_OF_ISOLATES', () => '8'));
    +  int specPerIsolate =
    +      int.parse(env.getOrElse('SPEC_PER_ISOLATE', () => '4'));
    +  int specExpireDuration =
    +      int.parse(env.getOrElse('SPEC_EXPIRE_DURATION', () => '300'));
    +} catch (e) {
    +  logger.severe('Error parsing environment variables: $e');
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for environment variable parsing enhances robustness by ensuring that parsing errors are logged, which aids in debugging and maintaining the application.

    8

    @leonardocustodio leonardocustodio merged commit 659aed3 into master Sep 24, 2024
    1 check passed
    @leonardocustodio leonardocustodio deleted the PLA-2013v2 branch September 24, 2024 16:05
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants