Conversation
62ed83f to
4ee68c8
Compare
| #[test] | ||
| fn empty_config_works() { |
There was a problem hiding this comment.
I'm not sure what is the intention of the original test, should we delete it?
Original PR:
https://github.com/foundry-rs/starknet-foundry/pull/1911/changes
There was a problem hiding this comment.
I guess just a sanity check, leaving this up to you whether to keep it or not
23de475 to
40addaf
Compare
24224a4 to
93b4912
Compare
| ))); | ||
| } | ||
| // Note: this is potentially unreachable: `get_or_create_global_config_path` should always return dir with existing config file. | ||
| // TODO: (#3436) remove this if missing global config becomes an error |
There was a problem hiding this comment.
I'm not sure if the linked issue is accurate here as it seems very general. Also what is the actual action item here, to decide whether missing global config file should be an error?
There was a problem hiding this comment.
Also what is the actual action item here, to decide whether missing global config file should be an error?
Previously, function get_global_config_path actually didn't only get the config path, but also created it, if missing. That's why I renamed it.
The original logic though allowed us to continue, even if global config is missing for some reason.
The reason for my comment is that described situation ("global config is missing") shouldn't really happen, at least in normal (non-error) circumstances, since we always create the dir instead.
There are some cases where it can:
- Missing
HOMEdir on the os - Some fs permission issues, disallowing us from creating config file
So the question is:
- Should we proceed execution in these cases, as was done before?
- Or should we treat these as hard errors?
- If we treat them as hard errors, then all these
global_maybe_config == MaybeConfig::NoFilecases become effectively unreachable. - If we keep it as it, we:
- treat them as errors when unable to resolve requested
--profile - proceed execution with a warning otherwise
- treat them as errors when unable to resolve requested
I think I attached the wrong issue though, it was planned to be handled under now closed #4184
Since this is getting very confusing, I suggest we either create a new issue, or (even better imo) decide on the final approach now 😄
There was a problem hiding this comment.
As discussed offline I'd lean towards a hard error, but given that so far we proceeded in such cases it also makes perfect sense to only display a warning. Both options work for me, let's choose one of these and address it in scope of this or next PR
| #[test] | ||
| fn empty_config_works() { |
There was a problem hiding this comment.
I guess just a sanity check, leaving this up to you whether to keep it or not
793e730 to
1978e97
Compare
- regular `runner` uses tempdir (so effectively empty global config; previously: dependend on local machine) - allows overriding global config dir (previously: no mechanism to test global config)
Co-authored-by: ddoktorski <45050160+ddoktorski@users.noreply.github.com>
…hen `local.profile` is missing
a697315 to
6894e2e
Compare
<!-- Reference any GitHub issues resolved by this PR --> ## Stack - #4175 - #4205 - #4249 - #4234 ## Introduced changes <!-- A brief description of the changes --> Allow configuring scarb profile separately from sncast config: - `--scarb-profile` in cli - `scarb-profile` in `snfoundry.toml` ## Checklist <!-- Make sure all of these are complete --> - [ ] Linked relevant issue - [x] Updated relevant documentation - [ ] Added relevant tests - [x] Performed self-review of the code - [x] Added changes to `CHANGELOG.md`
<!-- Reference any GitHub issues resolved by this PR --> ## Stack - #4175 - #4205 - #4249 - #4234 ## Introduced changes <!-- A brief description of the changes --> Allow configuring scarb profile separately from sncast config: - `--scarb-profile` in cli - `scarb-profile` in `snfoundry.toml` ## Checklist <!-- Make sure all of these are complete --> - [ ] Linked relevant issue - [x] Updated relevant documentation - [ ] Added relevant tests - [x] Performed self-review of the code - [x] Added changes to `CHANGELOG.md`
<!-- Reference any GitHub issues resolved by this PR --> Towards foundry-rs#4027 ## Stack - foundry-rs#4175 - foundry-rs#4205 - foundry-rs#4249 - foundry-rs#4234 ## Introduced changes <!-- A brief description of the changes --> #### Outline This is initial refactor of cast config handling. It aims to make the logic easier to reason about, removes tricky defaults resolution (using `None` instead), removes manual raw string approach config construction, and paves way for fixes introduced in foundry-rs#4205 Subsequent PR introduces more significant changes that change how configs are actually handled in more breaking way in attempt to address various issues, while this one mainly tries to keep current logic intact. #### Changes - Add `PartialCastConfig` with all fields having `None` as `Default` value. - Create global, local, cli `PartialCastConfig`. Then combine them (add `Override` trait for config and it's subcomponents) - Replace raw string approach to config in `add_created_profile_to_configuration` ## Checklist <!-- Make sure all of these are complete --> - [x] Linked relevant issue - [ ] Updated relevant documentation - [ ] Added relevant tests - [x] Performed self-review of the code - [ ] Added changes to `CHANGELOG.md` --------- Co-authored-by: Maksymilian Kowalski <126796018+MKowalski8@users.noreply.github.com>
Closes #3811
Closes #4027
Stack
sncast --profile#4249Summary
This PR fixes the actual bugs & issues I've been able to find, and changes the sncast handling logic in a more significant way
Introduced changes
Checklist
CHANGELOG.md