Testing CI for moving where the IPFS node is init #329
Conversation
| ipfsNode, err := createIpfsNode(config, true, logger) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create IPFS node: %w", err) | ||
| } | ||
|
|
||
| ipfsAPI, err := coreapi.NewCoreAPI(ipfsNode) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create an instance of the IPFS core API: %w", err) | ||
| } |
There was a problem hiding this comment.
This looks right to me. But I'm wondering if we shouldn't just pass in a CoreAPIobject into NewNode instead. Then it would be easier to e.g. use other mechanisms to use IPFS. Initialization would need to happen outside of Node.
Would also like to hear what @Wondertan thinks about this.
There was a problem hiding this comment.
I like that idea better than the current implementation
There was a problem hiding this comment.
Totally agree. Having dependency initialized outside of a component is also a good practice, which for our case would allow more flexibility to allow users to use custom IPFS nodes until we decide to fully absorb IPFS(not just embed) by using its modules directly. The latter may be possible in the future as an optimization vector.
There was a problem hiding this comment.
@liamsi, @evan-forbes, but how do we then ensure that a custom IPFS node includes the needed plugin(s)? That is not clear for me now and may require some investigation. I think we can still do the proposed changed on a code level for now(pass CoreAPI to NewNode), but still not give a user ability to use a custom one, until we understand how we can ensure that, as doing that now would be a waste of time.
@liamsi, if you haven't started this yet as part of the cleanup, I can include this change in my PR with API exposure to HTTP.
There was a problem hiding this comment.
@liamsi, if you haven't started this yet as part of the cleanup, I can include this change in my PR with API exposure to HTTP
I have started it only in the sense that I've extracted two methods to a file for initialization and for the creation of the node. The changes that are missing is to move the creation out of node.OnStart (and similarly out of the light client constructor - but that is not a concern on master yet).
Feel free to use the changes from the light client mvp or start from scratch. It's relatively minimal changes anyways.
Regarding the plugins:
That code also exist on master (only at a different place).
I'm not sure how to assert that the plugin was loaded if we use another approach.
| @@ -687,7 +706,7 @@ func NewNode(config *cfg.Config, | |||
| // If an address is provided, listen on the socket for a connection from an | |||
| // external signing process. | |||
| if config.PrivValidatorListenAddr != "" { | |||
| // FIXME: we should start services inside OnStart | |||
| // FIXME: we should start services inside rt | |||
| return nil, err | ||
| } | ||
|
|
||
| ipfsNode, err := createIpfsNode(config, true, logger) |
There was a problem hiding this comment.
If it's constantly true, maybe change then createIpfsNode?
| ipfsNode, err := createIpfsNode(config, true, logger) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create IPFS node: %w", err) | ||
| } | ||
|
|
||
| ipfsAPI, err := coreapi.NewCoreAPI(ipfsNode) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create an instance of the IPFS core API: %w", err) | ||
| } |
There was a problem hiding this comment.
Totally agree. Having dependency initialized outside of a component is also a good practice, which for our case would allow more flexibility to allow users to use custom IPFS nodes until we decide to fully absorb IPFS(not just embed) by using its modules directly. The latter may be possible in the future as an optimization vector.
|
While this was only a test and never meant to be merged, it is no longer needed thanks to #334 |
Description
This PR's purpose is to test how the CI handles moving when and where the IPFS node is initiated. This PR should not be merged yet, and will likely conform to whatever #323 and #312 end up doing
Closes: #XXX