Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: upgrademonitor #2922
feat: upgrademonitor #2922
Changes from 26 commits
98fdddb
2a0239a
cad89ce
11c7915
26b5eb3
57a1444
97035a7
3034773
b31244e
c49f4d7
fca19d9
338cc72
57774e1
5b9cf51
bb1d998
8795a2d
0a5a20e
de4dd31
fbe254f
1c9635e
dc50ab6
c4ad39f
ab70eff
4a76d26
d0e973a
eff04b2
766516a
4c612f1
0ec3969
7271be8
50fcc9a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The
cobra.Command
setup looks correct, and theRunE
function is well-structured to establish a gRPC connection, create a ticker for polling, and handle the upgrade monitoring logic. However, there are a few points to consider:The use of
insecure.NewCredentials()
in line 18 is potentially concerning from a security perspective. It's important to ensure that this is acceptable for the intended use case and that any risks are mitigated.The
RunE
function will exit after publishing a transaction when the network is upgradeable (line 43). This behavior should be clearly documented, and it should be confirmed that this is the desired behavior, as it means the tool will not continue to monitor after an upgrade is detected and handled.The logging in lines 34 and 42 is directly to stdout. Consider using a logging framework that can be configured for different environments and verbosity levels.
Error handling in lines 19 and 39 is straightforward, but it might be beneficial to log additional context or perform cleanup before returning the error.
The infinite loop starting at line 27 will run indefinitely unless an upgrade is detected. Ensure that there is a way to gracefully shut down the application if needed, such as by handling a termination signal.
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.
What was the rationale for this having it's own go.mod?
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.
hmm upgrademonitor is distinct from celestia-app in that the binary will be used independently of celestia-app. A distinct go.mod allows upgrademonitor to manage its own dependencies.
Your question made me realize that we could even extract upgrademonitor entirely to its own repo.
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.
If we want it in a stand alone repo: celestiaorg/upgrade-monitor#1