Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Conversation

@2501babe
Copy link
Contributor

@2501babe 2501babe commented Aug 3, 2022

fixes a regression introduced in #3166. prior to that, the config argument would default to the value of solana_cli_config::CONFIG_FILE, and loading the config would fall back to a default if it failed. changing the to an error protected against a missing file provided by --config, but also errored in the default case if they lacked the default file

this change distinguishes the three cases, erroring if the user provided a bad --config option, but falling back to defaults if they didnt

@2501babe 2501babe requested a review from joncinque August 3, 2022 17:42
@mergify mergify bot added the community Community contribution label Aug 3, 2022
@2501babe 2501babe removed the community Community contribution label Aug 3, 2022
@mergify mergify bot added the community Community contribution label Aug 3, 2022
joncinque
joncinque previously approved these changes Aug 3, 2022
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks

@mergify mergify bot dismissed joncinque’s stale review August 3, 2022 18:08

Pull request has been modified.

@joncinque joncinque removed the community Community contribution label Aug 3, 2022
@joncinque
Copy link
Contributor

of course it fails right before your new test... you can rebase on top of #3425, if you want to take your chances

@joncinque
Copy link
Contributor

#3425 landed, so if you don't mind rebasing once more, we can be sure that this is good to go

@2501babe
Copy link
Contributor Author

2501babe commented Aug 3, 2022

taking the cowards way out and deleting the test i attempted to add, it turns out spl-token address (and, in fact, every non-help command) attempts to load the signer keypair from its expected path and fails if its not there, and i cant tell it to use a different path in a test with a non-default config because thats... what the config does. and i dont want to write a key out to the default location and pollute a shared resource, if later tests end up depending on something else being there

there should be more comprehensive integration tests for the binary (pre this patch it crashed unconditionally on my machine) but thats a different thing imo

@joncinque
Copy link
Contributor

I think #3293 will at least address the config issue. Definitely happy to come up with a better testing strategy!

@2501babe 2501babe merged commit abc63ad into solana-labs:master Aug 3, 2022
@2501babe 2501babe deleted the config-fix branch August 3, 2022 21:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants