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

refactor(nodebuilder/header): get rid of InitStore #2678

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Sep 8, 2023

Not needed anymore

@renaynay renaynay added the kind:misc Attached to miscellaneous PRs label Sep 8, 2023
@renaynay renaynay self-assigned this Sep 8, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2678 (a37e634) into main (d803cf8) will decrease coverage by 0.12%.
Report is 3 commits behind head on main.
The diff coverage is 23.07%.

@@            Coverage Diff             @@
##             main    #2678      +/-   ##
==========================================
- Coverage   51.48%   51.36%   -0.12%     
==========================================
  Files         159      159              
  Lines       10677    10676       -1     
==========================================
- Hits         5497     5484      -13     
- Misses       4707     4715       +8     
- Partials      473      477       +4     
Files Changed Coverage Δ
nodebuilder/header/module.go 52.63% <ø> (+4.10%) ⬆️
nodebuilder/header/constructors.go 50.00% <16.66%> (-8.00%) ⬇️
nodebuilder/testing.go 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

utACK. Have you tested locally?

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

My intuition says me like there might be something subtle we miss by doing so. I clearly remember we had to do it, but don't remember why.

Does it works properly on non and initialised node node? Also, the test change is suspicious

@renaynay
Copy link
Member Author

renaynay commented Sep 8, 2023

@Wondertan Yes works for inited and non-inited nodes.

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

It seems like there is no blocker anymore for this simplification.
Thanks for this @renaynay!

@renaynay renaynay merged commit 6cc54e7 into celestiaorg:main Sep 11, 2023
13 of 16 checks passed
@renaynay renaynay deleted the get-rid-of-him branch September 11, 2023 13:48
renaynay added a commit to renaynay/celestia-node that referenced this pull request Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:misc Attached to miscellaneous PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants