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

goal: Add initialize option to 'goal node catchup'. #5754

Merged
merged 9 commits into from
Sep 26, 2023

Conversation

winder
Copy link
Contributor

@winder winder commented Sep 25, 2023

Summary

Add new parameter from #5752 to goal node catchup.

Minor refactoring to command to try and simplify the operations. In particular things I didn't like:

  • referencing args[0] without saying anywhere that it was an optional positional argument.
  • the catchup function references global abort flag, which is also referenced in the cobra function.
  • multiple client objects created.

Test Plan

Manually tested a bunch of permutations:

goal node catchup
goal node catchup -x
goal node catchup --force
goal node catchup --force -i 100000000
goal node catchup -i 100000000
goal node catchup -i 1000
goal node catchup -i 1000 --force
goal node catchup --force 
goal node catchup 29900000#K35DLY2QEMR6LPL6GOM5XR4ZVDYBNQ34TOWZVOPPL6PHSQGJJXHA
goal node catchup -i 100000000 29900000#K35DLY2QEMR6LPL6GOM5XR4ZVDYBNQ34TOWZVOPPL6PHSQGJJXHA
goal node catchup -i 100000000 --force 29900000#K35DLY2QEMR6LPL6GOM5XR4ZVDYBNQ34TOWZVOPPL6PHSQGJJXHA
goal node catchup -i 10000 29900000#K35DLY2QEMR6LPL6GOM5XR4ZVDYBNQ34TOWZVOPPL6PHSQGJJXHA --force
goal node catchup -i 10000 --force 29900000#K35DLY2QEMR6LPL6GOM5XR4ZVDYBNQ34TOWZVOPPL6PHSQGJJXHA --force

@winder winder self-assigned this Sep 25, 2023
cmd/goal/node.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #5754 (5226539) into master (17871a3) will decrease coverage by 0.01%.
The diff coverage is 4.76%.

@@            Coverage Diff             @@
##           master    #5754      +/-   ##
==========================================
- Coverage   55.45%   55.44%   -0.01%     
==========================================
  Files         473      473              
  Lines       66683    66682       -1     
==========================================
- Hits        36979    36972       -7     
- Misses      27184    27189       +5     
- Partials     2520     2521       +1     
Files Coverage Δ
libgoal/libgoal.go 2.44% <0.00%> (+0.01%) ⬆️
cmd/goal/node.go 12.65% <5.55%> (+0.18%) ⬆️

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@winder winder marked this pull request as ready for review September 26, 2023 15:19
@winder winder requested a review from jannotti September 26, 2023 15:20
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

The code LGTM, the new param name - maybe there are better options.

cmd/goal/node.go Show resolved Hide resolved
@winder winder merged commit ff82655 into algorand:master Sep 26, 2023
@winder winder deleted the will/goal-catchup-init branch September 26, 2023 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants