Skip to content

Conversation

@Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Jan 8, 2024

This PR aims at resolving state file path resolution
Closes ORC-46

Discussion

Today we have 2 variables to set the state file path :

  • CW_ORCH_STATE_FOLDER for the folder of the state file
  • STATE_FILE for the file name

This is too much. I propose to remove the first one and keep only the latter and follow the rules :
folder/file.json is .cw-orchestrator/folder/file.json

./folder/file.json is call_dir/folder/file.json

/usr/var/file.json is /usr/var/file.json

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jan 8, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c77fcfd
Status:🚫  Build failed.

View logs

@Buckram123
Copy link
Contributor

keep only the latter and follow the rules : folder/file.json is .cw-orchestrator/folder/file.json

./folder/file.json is call_dir/folder/file.json

Not sure how I feel about it. For me it looks messier than ability to set CW_ORCH_STATE_FOLDER, the only rule I would keep is: absolute path is absolute path

@Buckram123
Copy link
Contributor

Buckram123 commented Jan 8, 2024

The weirdness comes at the point that we decide to append cw-orch path without any additional input. What if we use something like shellexpand::full for this and set default CW_ORCH_STATE_FOLDER env instead
So the rules would be:

  • folder/file.json is folder/file.json

  • $CW_ORCH_STATE_FOLDER/folder/file.json is ~/.cw-orchestrator/folder/file.json

  • $USR_ENV/file.json is /foo/bar/file.json

  • ./folder/file.json is ./folder/file.json

  • /usr/var/file.json is /usr/var/file.json

@Kayanski thoughts

@Kayanski
Copy link
Contributor Author

Kayanski commented Jan 8, 2024

The weirdness comes at the point that we decide to append cw-orch path without any additional input. What if we use something like shellexpand::full for this and set default CW_ORCH_STATE_FOLDER env instead So the rules would be:

* `folder/file.json` is `folder/file.json`

* `$CW_ORCH_STATE_FOLDER/folder/file.json` is `~/.cw-orchestrator/folder/file.json`

* `$USR_ENV/file.json` is `/foo/bar/file.json`

* `./folder/file.json` is `./folder/file.json`

* `/usr/var/file.json` is `/usr/var/file.json`

@Kayanski thoughts

We chose to use ~/.cw-orchestrator by default to make sure that people don't need to be in a specific folder to run their scripts.
For instance, when execution cargo run --bin daemon_test in the root or in the cw-orch folder would produce 2 different files which is VERY bad behavior.
So we decided that using a ~/.cw-orchestrator folder was a good solution to this issue.

With ./ specific behavior, we still wanted the user to use files in the current dir if they really want it and without "./" to be able to use the non-error behavior

Now, we need to decide what to do for paths. Using $VARIABLE inside env files is very error-prone no ? Even if shellexpand covers it

Copy link
Contributor

@Buckram123 Buckram123 left a comment

Choose a reason for hiding this comment

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

Not sure how ../ would be handled. Agree it's much cleaner!

Comment on lines 120 to 136
if env_file_path
.components()
.map(|comp| comp.as_os_str().to_owned().into_string().unwrap())
.next()
== Some(".".to_string())
{
let current_dir = std::env::current_dir()?;
let actual_relative_path = env_file_path.strip_prefix("./")?;
current_dir.join(actual_relative_path)
} else {
let state_folder = default_state_folder()?;

// We need to create the default state folder if it doesn't exist
std::fs::create_dir_all(state_folder.clone())?;

state_folder.join(env_file_path)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder how it would handle ../

@codecov
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4cef88c) 65.6% compared to head (c77fcfd) 66.0%.

Additional details and impacted files
Files Coverage Δ
cw-orch-daemon/src/error.rs 14.2% <ø> (ø)
cw-orch-daemon/src/state.rs 85.7% <100.0%> (+5.3%) ⬆️
packages/cw-orch-core/src/env.rs 83.5% <100.0%> (+4.1%) ⬆️

@Kayanski Kayanski merged commit 5cbb161 into main Jan 11, 2024
@Kayanski Kayanski deleted the nicolas/orc-46-fix-broken-state-file-path-resolution branch January 11, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants