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(cmd): pass Start options; add WithFlagSet option #2950

Merged
merged 7 commits into from
Nov 23, 2023

Conversation

tuxcanfly
Copy link
Contributor

@tuxcanfly tuxcanfly commented Nov 21, 2023

Fixes #2936

This PR updates the Start command to accept a variadic number of funcs which can be used to customise the command.

This allows the caller to hook into and modify the command, for example to add a PreRun hook, as required for celestia-da.

With this PR in place, the celestia-da requirement, which needs to add some required flags and add a PreRun hook can be satisfied as:

	bridgeCmd := bridge.NewCommand(WithPreRun())
	lightCmd := light.NewCommand(WithPreRun())
	fullCmd := full.NewCommand(WithPreRun())

Ideally this can be applied for other commands Init, Auth as well, but it's not required for the current requirement.

@github-actions github-actions bot added the external Issues created by non node team members label Nov 21, 2023
@tuxcanfly tuxcanfly added kind:feat Attached to feature PRs area:cli and removed external Issues created by non node team members labels Nov 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2023

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (d1883d6) 51.43% compared to head (4a2e37e) 50.87%.

Files Patch % Lines
cmd/node.go 0.00% 65 Missing ⚠️
cmd/util.go 0.00% 5 Missing ⚠️
cmd/start.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2950      +/-   ##
==========================================
- Coverage   51.43%   50.87%   -0.56%     
==========================================
  Files         170      168       -2     
  Lines       10925    10937      +12     
==========================================
- Hits         5619     5564      -55     
- Misses       4807     4873      +66     
- Partials      499      500       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/celestia/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

personally would like to give this another refactor, which might be out of scope for this PR, unless this refactoring creates an external contract with what you are doing in celestia-da? Otherwise, this is a good first step to making it a bit nicer to work with, even if some of the indirection of what gets passed in and around is too clever for me, i think fix some of the ordering nits i made, and clarify if passing bridge.WithSubCommands is correct or not and lets roll

cmd/celestia/bridge/bridge.go Outdated Show resolved Hide resolved
cmd/celestia/bridge/bridge.go Outdated Show resolved Hide resolved
ramin
ramin previously approved these changes Nov 23, 2023
cmd/celestia/main.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Nov 23, 2023
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

cmd/node.go Outdated Show resolved Hide resolved
cmd/node.go Outdated Show resolved Hide resolved
@ramin ramin enabled auto-merge (squash) November 23, 2023 16:47
@ramin ramin merged commit dd3b8e1 into celestiaorg:main Nov 23, 2023
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:cli kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: allow customizable subcommands
4 participants