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.

Sign up for GitHub

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

preserve helper logs on bounce #87

Merged
merged 3 commits into from
Sep 26, 2024
Merged

preserve helper logs on bounce #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8963aa4
preserve helper logs on bounce
eriktaubeneck Sep 25, 2024
3e7f011
add = to nextjs version
eriktaubeneck Sep 25, 2024
ef519b6
pylint error name change
eriktaubeneck Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion etc/start_helper_sidecar.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ helper_port=$5
sidecar_port=$6
identity=$7

nohup draft run-helper-sidecar --config_path "$config_path" --root_path "$root_path" --helper_domain "$helper_domain" --sidecar_domain "$sidecar_domain" --helper_port "$helper_port" --sidecar_port "$sidecar_port" --identity "$identity" > .draft/logs/helper_sidecar.log 2>&1 & echo $! > $pid_file
log_file=".draft/logs/helper_sidecar_$(date "+%Y-%m-%d_%H-%M-%S").log"
nohup draft run-helper-sidecar --config_path "$config_path" --root_path "$root_path" --helper_domain "$helper_domain" --sidecar_domain "$sidecar_domain" --helper_port "$helper_port" --sidecar_port "$sidecar_port" --identity "$identity" > "$log_file" 2>&1 & echo $! > $pid_file
2 changes: 1 addition & 1 deletion server/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion server/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"chartjs-adapter-spacetime": "^1.0.1",
"clsx": "^2.0.0",
"haikunator": "^2.1.2",
"next": "14.2.11",
"next": "=14.2.11",
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the trade-offs of exact version pinning

The change from "next": "^14.2.11" to "next": "=14.2.11" locks the Next.js version to exactly 14.2.11. While this ensures consistency across environments, it may prevent automatic security updates and bug fixes.

Consider these alternatives:

  1. Use a tilde (~) instead of an equals sign to allow patch updates: "next": "~14.2.11"
  2. Keep the caret (^) to allow minor updates, but use a package lock file to ensure consistency.
  3. If exact pinning is necessary, implement a process to regularly check for and manually update to new versions.

Could you explain the reasoning behind this exact version pinning? If it's to address a specific issue, consider documenting it in a comment.

"octokit": "^3.1.2",
"pm2": "^5.4.1",
"react": "^18",
Expand Down
6 changes: 4 additions & 2 deletions sidecar/app/routes/start.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ def demo_logger(
return {"message": "Process started successfully", "query_id": query_id}


# pylint: disable=too-many-arguments
# pylint: disable=too-many-positional-arguments
Comment on lines +64 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring to reduce the number of arguments

While adding pylint disable comments addresses the immediate linter warnings, it might be beneficial to consider refactoring the function to reduce the number of arguments. This could improve the function's maintainability and readability.

Potential refactoring options:

  1. Group related parameters into a configuration object or dataclass.
  2. Use keyword-only arguments to make the function calls more explicit.
  3. Consider if some of these parameters could be set as default configurations elsewhere.

Example refactoring:

from dataclasses import dataclass

@dataclass
class IPAHelperConfig:
    gate_type: str
    stall_detection: bool
    multi_threading: bool
    disable_metrics: bool
    reveal_aggregation: bool

def start_ipa_helper(
    query_id: str,
    commit_hash: str,
    config: IPAHelperConfig,
    background_tasks: BackgroundTasks,
    request: Request
):
    # Function implementation

This approach would reduce the number of direct arguments and make the function signature cleaner.

@router.post("/ipa-helper/{query_id}")
def start_ipa_helper(
query_id: str,
Expand All @@ -73,7 +75,6 @@ def start_ipa_helper(
background_tasks: BackgroundTasks,
request: Request,
):
# pylint: disable=too-many-arguments
query_manager = request.app.state.QUERY_MANAGER
check_capacity(query_manager)

Expand Down Expand Up @@ -153,6 +154,8 @@ def iterfile():
)


# pylint: disable=too-many-arguments
# pylint: disable=too-many-positional-arguments
Comment on lines +157 to +158
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring to reduce the number of arguments

Similar to the start_ipa_helper function, it would be beneficial to refactor this function to reduce the number of arguments. This can improve maintainability and readability of the code.

Suggested refactoring approach:

  1. Group related parameters into a configuration object or dataclass.
  2. Use keyword-only arguments for clarity.

Example refactoring:

from dataclasses import dataclass

@dataclass
class IPAQueryConfig:
    size: int
    max_breakdown_key: int
    max_trigger_value: int
    per_user_credit_cap: int
    malicious_security: bool

def start_ipa_query(
    query_id: str,
    commit_hash: str,
    config: IPAQueryConfig,
    background_tasks: BackgroundTasks,
    request: Request
):
    # Function implementation

This refactoring would simplify the function signature and make it easier to add or modify parameters in the future without changing the function signature.

@router.post("/ipa-query/{query_id}")
def start_ipa_query(
query_id: str,
Expand All @@ -165,7 +168,6 @@ def start_ipa_query(
background_tasks: BackgroundTasks,
request: Request,
):
# pylint: disable=too-many-arguments
query_manager = request.app.state.QUERY_MANAGER
check_capacity(query_manager)

Expand Down
4 changes: 2 additions & 2 deletions sidecar/cli/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def start_traefik_local_command(
return Command(cmd=cmd, env=env)


# pylint: disable=too-many-arguments
# pylint: disable=too-many-positional-arguments
@cli.command
@click.option(
"--config_path",
Expand Down Expand Up @@ -164,7 +164,7 @@ def run_helper_sidecar(
start_commands_parallel([sidecar_command, traefik_command])


# pylint: disable=too-many-arguments
# pylint: disable=too-many-positional-arguments
@cli.command
@click.option(
"--config_path",
Expand Down
Loading