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

share/discovery: make constants configurable #930

Merged
merged 5 commits into from
Jul 25, 2022

Conversation

vgonkivs
Copy link
Member

Resolves #929

@vgonkivs vgonkivs added area:config CLI and config area:p2p area:networking kind:misc Attached to miscellaneous PRs labels Jul 21, 2022
@vgonkivs vgonkivs self-assigned this Jul 21, 2022
@vgonkivs vgonkivs changed the title share/discovery: make constant configurable share/discovery: make constants configurable Jul 21, 2022
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

nits otherwise lgtm

node/config_opts.go Outdated Show resolved Hide resolved
node/services/service.go Outdated Show resolved Hide resolved
node/tests/p2p_test.go Outdated Show resolved Hide resolved
service/share/discovery.go Show resolved Hide resolved
service/share/discovery.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs force-pushed the add_config_for_discovery branch 2 times, most recently from 58829fa to e2ac890 Compare July 22, 2022 07:59
@vgonkivs vgonkivs requested a review from renaynay July 22, 2022 07:59
@vgonkivs vgonkivs force-pushed the add_config_for_discovery branch from e2ac890 to 5d1058e Compare July 22, 2022 08:00
@vgonkivs
Copy link
Member Author

Test failed because of #931

@vgonkivs vgonkivs requested a review from Bidon15 July 22, 2022 10:26
@vgonkivs vgonkivs force-pushed the add_config_for_discovery branch from 5d1058e to 9d8b383 Compare July 22, 2022 14:30
@vgonkivs vgonkivs force-pushed the add_config_for_discovery branch from 9d8b383 to 3d0c6fe Compare July 22, 2022 14:32
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.

LGTM, but ideally, we would avoid making the discovery constructor public and pass configs with share pkg-specific options. That is, options on the node would trigger options on the share, but this has very low prio

node/services/config.go Show resolved Hide resolved
service/share/set.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member Author

LGTM, but ideally, we would avoid making the discovery constructor public and pass configs with share pkg-specific options. That is, options on the node would trigger options on the share, but this has very low prio

Would you mind if I create an issue for that? @Wondertan

@Wondertan
Copy link
Member

Let's do

node/config_opts.go Outdated Show resolved Hide resolved
node/services/service.go Outdated Show resolved Hide resolved
node/services/service.go Outdated Show resolved Hide resolved
node/tests/p2p_test.go Show resolved Hide resolved
service/share/discovery.go Outdated Show resolved Hide resolved
service/share/discovery.go Show resolved Hide resolved
service/share/discovery.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs requested a review from renaynay July 25, 2022 11:05
@codecov-commenter
Copy link

Codecov Report

Merging #930 (ef406a9) into main (8eee5c9) will increase coverage by 0.07%.
The diff coverage is 73.49%.

@@            Coverage Diff             @@
##             main     #930      +/-   ##
==========================================
+ Coverage   58.70%   58.78%   +0.07%     
==========================================
  Files         128      128              
  Lines        7624     7681      +57     
==========================================
+ Hits         4476     4515      +39     
- Misses       2678     2696      +18     
  Partials      470      470              
Impacted Files Coverage Δ
node/config_opts.go 28.57% <0.00%> (-11.43%) ⬇️
service/share/discovery.go 34.72% <50.00%> (+0.87%) ⬆️
node/components.go 90.69% <100.00%> (ø)
node/services/config.go 51.51% <100.00%> (+4.84%) ⬆️
node/services/service.go 81.87% <100.00%> (+4.19%) ⬆️
service/share/full_availability.go 88.23% <100.00%> (ø)
service/share/light_availability.go 84.48% <100.00%> (ø)
service/share/testing.go 82.75% <100.00%> (+0.24%) ⬆️
cmd/flags_core.go 42.37% <0.00%> (-0.74%) ⬇️
header/core/listener.go 58.49% <0.00%> (+5.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

service/share/discovery.go Show resolved Hide resolved
@vgonkivs vgonkivs merged commit 2139ace into celestiaorg:main Jul 25, 2022
@vgonkivs vgonkivs deleted the add_config_for_discovery branch January 9, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config CLI and config area:networking area:p2p kind:misc Attached to miscellaneous PRs
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

share/discovery: make all related to discovery constants configurable
4 participants