[go: nahoru, domu]

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

(feat) modify start.sh command to use quickstart arguments #6566

Merged
merged 14 commits into from
Sep 21, 2023

Conversation

david-hummingbot
Copy link
Contributor

references issue - #6564

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:

Tests performed by the developer:

  • Cloned branch, compiled and started client using the modified ./start command
  • Test ./start command passing the -p password parameter (only works using environment variables)

Tips for QA testing:

@rapcmia rapcmia requested review from nikspz and removed request for rapcmia September 13, 2023 02:02
@nikspz
Copy link
Contributor
nikspz commented Sep 13, 2023
  • Test performed:
    • Cloned and installed feature branch
    • Created new text+number Pas11 password after ./start, passed successfully
    • Trying to pass using ./start -p Pas11 - Failed (Client asking for password)
    • Trying to pass using ./start -p 'Pas11' - Failed (Client asking for password)
    • Trying to pass using ./start Pas11 - Failed (Client asking for password)
    • Created paper.yml strategy
    • Trying to pass using ./start -f paper.yml -p Pas11 - Failed (Client asking for password)
    • set new env variable pass: export pass=Pas11
    • ./start -p pass - Client started successfully
    • ./start -f paper.yml -p pass - Client started successfully, strategy do not started
    • set new env variable strat: export CONFIG_FILE_NAME=paper.yml
    • ./start -f CONFIG_FILE_NAME - Typed password manually, strategy started successfully
    • ./start -p pass -f CONFIG_FILE_NAME - Client started successfully, strategy started successfully

Note: you need to set up env variables, simple password/strategy.yml string will fail

Copy link
Contributor
@cardosofede cardosofede left a comment

Choose a reason for hiding this comment

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

LGTM! good job @david-hummingbot

did you tried to pass the arguments?

@rapcmia rapcmia marked this pull request as draft September 18, 2023 05:25
@nikspz
Copy link
Contributor
nikspz commented Sep 20, 2023

Fresh installation. commit c52846e
Steps:
0. I have a variable set (stra="paper.yml"
and strat="paper.yml")

  1. Clone and install PR, source installation, ubuntu
  2. start the client with bin/hummingbot.py Set up password a
  3. exit client
  4. try to start the bot with ./start command

Actual: bot failed to start

Expected: bot starts and requests for password

6566 c52846e09d9c

errors.log
logs_hummingbot.log

@nikspz
Copy link
Contributor
nikspz commented Sep 20, 2023

Deleted all previously set variables and retested:

  • ./start -p a - success
  • ./start - success
  • created paper.yml
  • ./start -p a -f paper.yml - success
  • deleted password verification
  • set new apass11
  • ./start -p apass11 success

Steps:
0. set up password

  1. ./start -p apass (wrong password)

Actual:
no indication on wrong password / wrong strategy name
however errors.log contains info about it
errors.log

Expected:
output in the terminal for wrong password / wrong strategy name

6566

https://www.loom.com/share/ee3ccd788b95480ab216d28652255665

david-hummingbot and others added 7 commits September 20, 2023 23:59
- exit code 1 for invalid password
- exit code 2 for invalid config file
reverted change to using exit codes and instead start command will check the error logs and output the result in the terminal
@rapcmia rapcmia marked this pull request as ready for review September 21, 2023 10:18
Copy link
Contributor
@nikspz nikspz left a comment

Choose a reason for hiding this comment

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

  • test performed:
    • Cloned and installed
    • set up password a
    • ./start -p a - success
    • ./start - success
    • created paper.yml
    • ./start -p a -f paper.yml - success
    • deleted password verification
    • set new Pa11 password
    • ./start -p Pa11 success
    • ./start -p Pa1123 - incorrect password warning message showing
    • ./start -p Pa11 -p nostrategy.yml - invalid filename provided warning

@nikspz nikspz merged commit 2aa845c into development Sep 21, 2023
2 checks passed
@nikspz nikspz deleted the feat/use_quickstart_in_start_command branch September 21, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants