Remove app package
#4185
Remove app package
#4185
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR removes the app package by redistributing its responsibilities between the node and main packages to better align with their intended purposes. The main package handles process lifecycle management while the node package manages business logic wiring.
- Moves the
Appinterface andappstruct fromapppackage tonodepackage asRunner - Relocates process lifecycle functions (
Run) and ASCII header tomainpackage - Updates import statements and function calls to reflect the new package structure
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| node/runner.go | Converts app logic to Runner struct in node package, removing signal handling |
| main/main.go | Adds process lifecycle management and ASCII header from removed app package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -71,74 +50,24 @@ func New(config nodeconfig.Config) (App, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| n, err := node.New(&config, logFactory, log) | |||
| n, err := New(&config, logFactory, log) | |||
There was a problem hiding this comment.
The function call New(&config, logFactory, log) appears to be calling a function that doesn't exist in the current context. This was previously node.New() but the import for the node package was removed. This will cause a compilation error.
Copilot uses AI. Check for mistakes.
| os.Exit(Run(runner)) | ||
| } | ||
|
|
||
| const Header = ` _____ .__ .__ |
There was a problem hiding this comment.
nit: typically consts live at the top of the file
| \____|__ /\_/ (____ /____(____ /___| /\___ >___| /\___ > \\ | ||
| \/ \/ \/ \/ \/ \/ \/` | ||
|
|
||
| func Run(app *node.Runner) int { |
There was a problem hiding this comment.
I wonder if we even need this function - we only use it once anyways. Should we inline this into main?
| } | ||
|
|
||
| func New(config nodeconfig.Config) (App, error) { | ||
| func NewRunner(config nodeconfig.Config) (*Runner, error) { |
There was a problem hiding this comment.
The code in main owns how to actually start a node/when to shutdown... so I think this code belongs in main more than it belongs in node.
Taking a step back... I'm not sure why we needed the old App type to begin with (now called Runner). There seems to be a clear abstraction boundary between node (our business logic/an avax node), and main (the runner/how we spin up a node), but App/Runner doesn't seem to have a clear boundary of what it owns. I think it might be most reasonable to just in-line all of this code into main and remove Runner altogether, or keep the abstraction but unexport it and leave it under main/.
Why this should be merged
Issue #2308 - The app package defines a lot of stuff that should exist either in the node package or in main. main is already the abstraction for the application the user interacts with and node is the layer of our business logic - so app doesn't have a clear boundary.
How this works
Moves app/app.go logic to node and main packages, separated into files for node wiring (in node/) and process lifecycle (in main/).
How this was tested
CI, manual build test
Need to be documented in RELEASES.md?
No