-
Notifications
You must be signed in to change notification settings - Fork 40
fix for deployment app-tendermint logging issue with the same file op… #2357
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
base: main
Are you sure you want to change the base?
Conversation
419d85f
to
24dadaf
Compare
90e6b97
to
5e25cb7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2357 +/- ##
==========================================
+ Coverage 84.89% 84.92% +0.02%
==========================================
Files 225 225
Lines 16496 16501 +5
==========================================
+ Hits 14005 14013 +8
+ Misses 2491 2488 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I tried running with these changes and something is off...
Now the logs are some lines and then full of empty strings, could you check what's wrong here?
Attaching the logs here.
# commented out, cause deployment app needs some changes | ||
# and looks like its not mandatory to relfect changes in connections code | ||
# (connection.TendermintNode, tendermint.TendermintNode), # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to keep them identical even if it's not needed in the connections code, and let this test as it was.
My reasoning is that, its fine to comment it right now, but removing this test mean that in may slip other changes also in the future, which would need to be identical. So, letting the process unchanged will avoid problems in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I just realise! Why not do the same fix in the connections also? It would also be creating this bug too right?
except ImportError: | ||
from tendermint import TendermintNode, TendermintParams # type: ignore | ||
|
||
DEFAULT_LOG_FILE_MAX_BYTES = 50 * 1024 * 1024 # 50MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have this in deployments/Dockerfiles/tendermint/tendermint.py
4cf4a30
to
c799a48
Compare
7a8dcad
to
83a7797
Compare
368e6c9
to
9dac05f
Compare
9dac05f
to
745f49f
Compare
…ened
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
Fixes
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce? (A breaking change is a fix or feature that would cause existing functionality and APIs to not work as expected.)
Put an
x
in the box that appliesChecklist
Put an
x
in the boxes that apply.main
branch (left side). Also you should start your branch off ourmain
.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...