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

update script to launch timekeeper first #346

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented Oct 4, 2024

PR Type

enhancement


Description

  • Updated the script to prioritize handling the timekeeper node before other nodes.
  • Modified the .env file for the timekeeper node with the POT_EXTERNAL_ENTROPY value.
  • Improved logging to provide more detailed information about the timekeeper node operations.
  • Simplified and clarified the process for updating and starting farmer and RPC nodes.
  • add optional argument to disable updating the timekeeper in case of manual launching

Changes walkthrough 📝

Relevant files
Enhancement
manage_subspace.py
Prioritize timekeeper node in launch script and update .env handling

scripts/launch-nodes/manage_subspace.py

  • Updated the script to handle the timekeeper node first.
  • Modified .env file for the timekeeper with POT_EXTERNAL_ENTROPY.
  • Added logging for timekeeper node operations.
  • Simplified the process for handling farmer and RPC nodes.
  • +25/-41 

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

    @github-actions github-actions bot added the enhancement New feature or request label Oct 4, 2024
    Copy link

    github-actions bot commented Oct 4, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The error handling for SSH connections and operations on nodes seems incomplete. Consider implementing more robust error handling and recovery strategies, especially for network issues or permission errors that might occur during SSH operations.

    Logging Consistency
    The logging levels and messages are inconsistent. Ensure that the logging throughout the script is consistent and provides clear, actionable information, especially in error scenarios.

    Copy link

    github-actions bot commented Oct 4, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Improve resource management by using context managers for SSH connections

    Use context managers for handling SSH connections to ensure that connections are
    always closed properly.

    scripts/launch-nodes/manage_subspace.py [167-184]

    -client = ssh_connect(timekeeper_node['host'], timekeeper_node['user'], timekeeper_node['ssh_key'])
    -...
    -if client:
    -    client.close()
    +with ssh_connect(timekeeper_node['host'], timekeeper_node['user'], timekeeper_node['ssh_key']) as client:
    +    ...
    Suggestion importance[1-10]: 9

    Why: Using context managers for SSH connections ensures that resources are properly managed and connections are closed even if an error occurs, improving the code's reliability and maintainability.

    9
    Security
    Enhance security by validating args.pot_external_entropy before usage

    Validate the args.pot_external_entropy before using it to ensure it meets expected
    criteria.

    scripts/launch-nodes/manage_subspace.py [174]

    -modify_env_file(client, subspace_dir, release_version, pot_external_entropy=args.pot_external_entropy)
    +if validate_pot_external_entropy(args.pot_external_entropy):
    +    modify_env_file(client, subspace_dir, release_version, pot_external_entropy=args.pot_external_entropy)
    +else:
    +    logger.error("Invalid POT_EXTERNAL_ENTROPY provided.")
    Suggestion importance[1-10]: 8

    Why: Validating args.pot_external_entropy before using it can prevent potential misuse or errors, enhancing the security and reliability of the script. This is a significant improvement.

    8
    Enhancement
    Ensure consistent behavior by providing a default value for the --pot_external_entropy argument

    Add a default value for the --pot_external_entropy argument to ensure consistent
    behavior when the argument is not provided.

    scripts/launch-nodes/manage_subspace.py [141]

    -parser.add_argument('--pot_external_entropy', help='POT_EXTERNAL_ENTROPY value for all nodes')
    +parser.add_argument('--pot_external_entropy', default='default_value', help='POT_EXTERNAL_ENTROPY value for all nodes')
    Suggestion importance[1-10]: 7

    Why: Adding a default value for the --pot_external_entropy argument can prevent potential errors when the argument is not provided, ensuring consistent behavior. This is a useful enhancement for robustness.

    7

    @DaMandal0rian DaMandal0rian merged commit 8bbd1de into main Oct 7, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the fix-pot-bug-launch-script branch October 7, 2024 11:49
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants