refactor(v7): make dockerutil internal/ public

[Reecepbcups]

closes #869 (part 2)

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
interchaintest-docs	⬜️ Ignored (Inspect)	Visit Preview		May 1, 2024 6:38pm

[jtieri]

Before i actually review the changes i just wanted to ask, does it make sense for all of this code to become public?

I can see the argument for making the internal/dockerutil package public because there are actually a lot of scenarios where you may want lower level access to the containers or volumes for container lifecycle management or reading/writing data, but i don't necessarily see a case where the blockdb or mocktesting packages are useful for developers working outside of the interchaintest repo.

iirc mocktesting is used for internal testing inside interchaintest, so i can't see why anyone would need to import code from this package. I also can't think of a reason devs would need to do anything with blockdb if interchaintest is already handling all of the database specific logic for you e.g. connecting to DB, the schemas, extracting chain related data, inserting data, etc. I can actually imagine making the blockdb package public could lead to weird behavior if devs try to call certain functions and also have BlockDatabaseFile set in Interchain.Build, for example you could end up trying to call ConnectDB explicitly after the internal logic in interchaintest has already initialized a connection and this would lead to resource contention due to sqlite not supporting more than 1 open connection per process very elegantly.

tl;dr: I'm in favor of making the Docker related code public but the rest of it seems useless from the perspective of developers making use of the testing framework and i think it would also create additional footguns in the case of blockdb since this code wasn't written in a way that it should be invoked outside of internal interchaintest functionality.

[Reecepbcups]

These are good points. I was thinking developers could extend BlockDB, but this again could lead to more confusion when new contributors try using it directly improperly. Ill change this now

[jtieri]

These are good points. I was thinking developers could extend BlockDB, but this again could lead to more confusion when new contributors try using it directly improperly. Ill change this now

It's possible that someone may find a usecase for this but i think until we have a clear user story it makes sense to keep the package internal so we don't clutter the public API exposed by the framework and also don't introduce additional footguns for users.

I don't think many people even know about the blockdb feature, could be a good candidate for a future blog post or maybe just we just write some better docs around it.

[jtieri]

had the one comment but otherwise this looks good to me! i'll just approve now so you aren't blocked on me