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

feat(share)!: add functional options to share pkg #1798

Merged
merged 22 commits into from
May 8, 2023

Conversation

derrandz
Copy link
Contributor

@derrandz derrandz commented Feb 22, 2023

Overview

This PR introduces functional options to the share package and deprecates usage of default values directly in code, in favor of using parameterized values.

Breaking

This PR breaks the configuration. The on-disk configuration becomes (note that Share.Availability is only available for light nodes):

[Share]
  [Share.Availability]
    SampleAmount = 16
  [Share.Discovery]
    PeersLimit = 5
    AdvertiseInterval = "8h0m0s"

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@derrandz derrandz added enhancement New feature or request area:config CLI and config area:shares Shares and samples kind:feat Attached to feature PRs area:api Related to celestia-node API labels Feb 22, 2023
@derrandz derrandz self-assigned this Feb 22, 2023
@derrandz derrandz changed the title feat: add functional options to share pkg feat(share): add functional options to share pkg Feb 22, 2023
@derrandz derrandz requested a review from walldiss as a code owner March 7, 2023 15:31
@derrandz derrandz force-pushed the feat/share/functional-options branch from 6359309 to 41c8612 Compare March 7, 2023 15:39
@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2023

Codecov Report

Merging #1798 (41c8612) into main (915b7ae) will decrease coverage by 0.01%.
The diff coverage is 61.32%.

@@            Coverage Diff             @@
##             main    #1798      +/-   ##
==========================================
- Coverage   57.44%   57.44%   -0.01%     
==========================================
  Files         242      243       +1     
  Lines       16211    16391     +180     
==========================================
+ Hits         9313     9415     +102     
- Misses       5954     6019      +65     
- Partials      944      957      +13     
Impacted Files Coverage Δ
share/availability/options.go 47.10% <47.10%> (ø)
nodebuilder/share/config.go 70.00% <50.00%> (-5.00%) ⬇️
share/availability/cache/availability.go 82.60% <66.66%> (-4.90%) ⬇️
share/availability/discovery/discovery.go 54.68% <75.00%> (-1.67%) ⬇️
nodebuilder/share/constructors.go 93.75% <100.00%> (+0.89%) ⬆️
nodebuilder/share/module.go 93.75% <100.00%> (+0.70%) ⬆️
share/availability/full/availability.go 76.74% <100.00%> (+3.77%) ⬆️
share/availability/full/testing.go 100.00% <100.00%> (ø)
share/availability/light/availability.go 82.00% <100.00%> (+2.00%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Just a top level overview. Lets reorganise it into subpkgs and rebase on latest main.

share/availability.go Outdated Show resolved Hide resolved
share/availability/options.go Outdated Show resolved Hide resolved
share/availability/options.go Outdated Show resolved Hide resolved
share/availability/options.go Outdated Show resolved Hide resolved
nodebuilder/share/constructors.go Outdated Show resolved Hide resolved
share/availability.go Outdated Show resolved Hide resolved
share/availability.go Outdated Show resolved Hide resolved
share/availability.go Outdated Show resolved Hide resolved
@walldiss
Copy link
Member

@derrandz some conversations seems to be resolved by you, but there is no response, nor any changes applied. Are they resolved by mistake?

@derrandz
Copy link
Contributor Author

I resolved locally but ended up facing the import cycle issue and not pushing @walldiss so they are implemented but not pushed yet. Will unresolve til I push.

@derrandz derrandz force-pushed the feat/share/functional-options branch from a8ba5a3 to 9000f51 Compare March 15, 2023 18:36
@derrandz derrandz requested a review from walldiss March 15, 2023 18:38
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.

Please lint and refer to comments re errors.

nodebuilder/share/config.go Outdated Show resolved Hide resolved
nodebuilder/share/config.go Outdated Show resolved Hide resolved
nodebuilder/share/config.go Outdated Show resolved Hide resolved
nodebuilder/share/config.go Outdated Show resolved Hide resolved
share/availability/cache/options.go Outdated Show resolved Hide resolved
share/availability/cache/options.go Outdated Show resolved Hide resolved
share/availability/cache/options.go Outdated Show resolved Hide resolved
share/availability/cache/options.go Outdated Show resolved Hide resolved
share/availability/discovery/options.go Outdated Show resolved Hide resolved
share/availability/light/options.go Show resolved Hide resolved
@derrandz derrandz requested a review from renaynay March 20, 2023 12:00
Wondertan
Wondertan previously approved these changes May 3, 2023
distractedm1nd
distractedm1nd previously approved these changes May 4, 2023
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.

  • please check to ensure units + units+race are passing, looks like there's an error associated with types

nodebuilder/share/config.go Outdated Show resolved Hide resolved
share/availability/light/options.go Show resolved Hide resolved
share/availability/light/options.go Outdated Show resolved Hide resolved
share/availability/cache/availability.go Outdated Show resolved Hide resolved
nodebuilder/share/config.go Show resolved Hide resolved
nodebuilder/share/config.go Outdated Show resolved Hide resolved
nodebuilder/share/config.go Outdated Show resolved Hide resolved
share/availability/discovery/discovery.go Outdated Show resolved Hide resolved
share/availability/discovery/options.go Outdated Show resolved Hide resolved
share/availability/discovery/options.go Show resolved Hide resolved
nodebuilder/share/config.go Outdated Show resolved Hide resolved
nodebuilder/share/config.go Outdated Show resolved Hide resolved
share/availability/discovery/options.go Show resolved Hide resolved
share/availability/discovery/options.go Show resolved Hide resolved
@derrandz derrandz dismissed stale reviews from distractedm1nd and Wondertan via 89096ab May 4, 2023 11:49
@derrandz derrandz force-pushed the feat/share/functional-options branch from 89096ab to c8f713c Compare May 4, 2023 14:03
@derrandz derrandz force-pushed the feat/share/functional-options branch from c8f713c to 2045029 Compare May 4, 2023 14:04
Wondertan
Wondertan previously approved these changes May 4, 2023
nodebuilder/share/config.go Show resolved Hide resolved
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 took us a while but we finally updated the config

@renaynay renaynay enabled auto-merge (squash) May 8, 2023 08:41
@renaynay renaynay merged commit 7f556f0 into celestiaorg:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Related to celestia-node API area:config CLI and config area:shares Shares and samples enhancement New feature or request kind:break! Attached to breaking PRs kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants