refactor: parse manifest template to RawManifest, use toml_edit
#1333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mkenigs
reviewed
Apr 17, 2024
ysndr
reviewed
Apr 18, 2024
Co-authored-by: Matthew Kenigsberg <matthew@floxdev.com>
oxcabe
force-pushed
the
refactor/flox-init/toml-edit-replace
branch
from
1f8aa58
to
41631b9
Compare
|
Just a heads up, this is going to have merge conflicts with #1299 |
|
Should I finish this ASAP? |
|
I think honestly it would be better to wait till 1299 gets in and then deal with conflicts :/ |
|
The implementation design and approach taken could be reviewed. A few failing tests have to be checked before we consider this functionally correct, though. |
ysndr
previously requested changes
May 9, 2024
|
Thanks! I'll address them later on today 👍 |
Co-authored-by: Yannik Sander <7040031+ysndr@users.noreply.github.com>
## Proposed Changes Resolve any dots in paths when determining the environment name so that `init -d` works when the final path item is a single or double dot. We can't use `canonicalize` because it will return an error if the path doesn't exits which breaks our existing behaviour for init against a directory that doesn't yet exist: - https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize The following implementations all don't resolve a single `.` to a name: - https://doc.rust-lang.org/std/path/fn.absolute.html - https://docs.rs/path-absolutize/latest/path_absolutize/ - https://docs.rs/path-clean/latest/path_clean/ - https://github.com/rust-lang/cargo/blob/0.79.0/crates/cargo-util/src/paths.rs#L76-L109 So the `path-dedot` crate seemed like the most appropriate choice. I considered adding unit tests for this behaviour but the integration tests are actually very fast for this functionality, make it simpler to test changing current working directory in the context of running the CLI, and give us some reassurance that we haven't affected the downstream behaviour of creating an environment. Output of the new tests before the implementation was added: ✗ c2.1: `flox init` with `--dir .` will create an environment in current working directory. [37] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 115) `assert_success' failed ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root ✗ c2.1: `flox init` with `--dir ..` will create an environment in parent working directory. [39] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 125) `assert_success' failed /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test/other /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/flox-cli-tests-2WdbV0 ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root ## Release Notes Support `flox init -d` where the final part of the path is a `.` or `..`.
- Provide tokio to functions called by `spin_with_delay`
The catalog client uses `reqwest` which uses `tokio::time::sleep`. When
the catalog is used in combination with our `spin_with_delay` function,
this causes a panic:
```
there is no reactor running, must be called from the context of a Tokio
1.x runtime
```
Enter the tokio runtime context in the thread spawned by
`spin_with_delay` to avoid the panic.
- Add functional test for catalog install. Note that this is still
skipped since we're waiting for changes serverside
|
How should we approach unit tests? |
I would add a bunch of tests for I would probably add cases for:
|
mkenigs
reviewed
May 20, 2024
|
The newline broke some tests |
Co-authored-by: Matthew Kenigsberg <matthew@floxdev.com>
auto-merge was automatically disabled
May 21, 2024 19:58
Head branch was pushed to by a user without write access
Got it. I'll fix them. |
mkenigs
approved these changes
May 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
This PR replaces uses of
String::replacefor editing and populating placeholders inmanifest.tomlto usetoml_editinstead.Closes #1064.
Release Notes
N/A.