fix: Local-interchain cleanup #710
Conversation
Prevents needing to build the binary first.
| func init() { | ||
| rootCmd.AddCommand(newChainCmd) | ||
| } |
There was a problem hiding this comment.
I avoid init() functions at all costs. Even though this is common in Viper and co-located with the subcommand file, I think it's an anti-pattern. It hides order of operations and hides functionality in the main() function. IOW, I want to look at main() and know exactly what's happening.
| var ( | ||
| MakeFileInstallDirectory string | ||
| ) |
There was a problem hiding this comment.
Avoid global vars. In this case, wasn't needed at all.
There was a problem hiding this comment.
This is required for the ldflags in makefile to work I believe.
or does the var in get directory work properly when you make install. This is the reason it is a globalvar
ldflags = -X main.MakeFileInstallDirectory=$(CWD)
There are 3 install cases:
- the user
make installs, it sets the current install location in the system - ICTEST_HOME env is used
- neither of the above are true, and we set ictest to be at the user's $HOME/local-interchain location
This would remove the first which is the most common desire imo
| @@ -62,7 +63,7 @@ func SetupGenesisWallets(config *types.Config, chains []ibc.Chain) map[ibc.Chain | |||
| for _, coin := range amount { | |||
| additionalWallets[chainObj] = append(additionalWallets[chainObj], ibc.WalletAmount{ | |||
| Address: acc.Address, | |||
| Amount: coin.Amount.Int64(), | |||
| Amount: math.NewInt(coin.Amount.Int64()), | |||
There was a problem hiding this comment.
Ah nice. Yea this makes sense because this got merged first:
Really surprised CI didn't catch this??
There was a problem hiding this comment.
Oh one more note... around backporting.
That PR is not backported yet: #685
There was a problem hiding this comment.
Oh good catch. We should get that back ported. I can look at it.
Of course! Consider init functions harmful. They modify or create global state and their order of operations is not intuitive. |
There was a problem hiding this comment.
@DavidNix I reverted your change here to make the variable private (b87ff71). The reason this is public is for the makefile to build to the proper directory (static link within the bin at compile time)
I added a comment to make others aware of this in the code too
ldflags = -X main.MakeFileInstallDirectory=$(CWD)
...
.PHONY: install
install:
go install $(BUILD_FLAGS) ./cmd/local-ic ./interchainOther than that it LGTM
* Fix compiler error * Use go run in Makefile Prevents needing to build the binary first. * Remove unecessary init functions * Remove unecessary global var * Fix misspelling in var name * Revert "Remove unecessary global var" This reverts commit 20cf604. * Add comment for why InstallDir must be public --------- Co-authored-by: Reece Williams <[email protected]> (cherry picked from commit 5c4c2b6)
* Fix compiler error * Use go run in Makefile Prevents needing to build the binary first. * Remove unecessary init functions * Remove unecessary global var * Fix misspelling in var name * Revert "Remove unecessary global var" This reverts commit 20cf604. * Add comment for why InstallDir must be public --------- Co-authored-by: Reece Williams <[email protected]> (cherry picked from commit 5c4c2b6) Co-authored-by: David Nix <[email protected]>
Fixes one compiler error.