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

chore: change tradingview env param with prefix #616

Merged
merged 4 commits into from
Mar 27, 2023

Conversation

chrisleekr
Copy link
Owner

@chrisleekr chrisleekr commented Mar 27, 2023

Description

In the last release, new env params are added to allow setting trading view configurations. And there was an existing env param that was not following the naming conventions.

However, some env params conflict with the environment parameter in Kubernetes if the service name is ‘tradingview’, which is common to set.

It's good practice to follow previous naming conventions. In addition, prevent clashes by adding prefixes.

Impacted environment parameters:

  • TRADINGVIEW_HOST -> BINANCE_TRADINGVIEW_HOST
  • TRADINGVIEW_PORT -> BINANCE_TRADINGVIEW_PORT
  • TRADINGVIEW_LOG_LEVEL -> BINANCE_TRADINGVIEW_LOG_LEVEL

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

@chrisleekr chrisleekr added the bug Something isn't working label Mar 27, 2023
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (1c833c5) 100.00% compared to head (0585bc4) 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #616   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           91        91           
  Lines         3498      3498           
  Branches       607       607           
=========================================
  Hits          3498      3498           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chrisleekr chrisleekr merged commit 9bc89f0 into master Mar 27, 2023
@rando128
Copy link
Contributor

rando128 commented Apr 1, 2023

@chrisleekr, do we agree this will require a migration before releasing so the changes on custom-environment-variables.json get applied?

@chrisleekr
Copy link
Owner Author

@rando128 Yeah, I do agree in principle. But not sure how we make a migration for this change.
If you have any suggestions, I am very happy to listen.

Although since it's very customisable, I assume fewer traders will be impacted.
Please let me know if you have a good idea.

@rando128
Copy link
Contributor

rando128 commented Apr 3, 2023

I've applied a new flush configuration migration like this one and it seems to work.

@chrisleekr
Copy link
Owner Author

@rando128 Ah, that was what you meant!

Yeah, that is a very good point. I will add the flush configuration migration.

@chrisleekr chrisleekr deleted the chore/change-tradingview-config branch May 4, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants