Skip to content

Commit

Permalink
Var sc bugfix when used in post and pre exists (#20)
Browse files Browse the repository at this point in the history
  • Loading branch information
zakstucke authored Feb 27, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 8072c24 commit 53103e4
Showing 5 changed files with 84 additions and 14 deletions.
4 changes: 2 additions & 2 deletions py_rust/src/config/tasks.rs
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ use crate::{
},
};

static IN_TASK_ENV_VAR: &str = "ZETCH_IN_TASK";
pub static IN_TASK_ENV_VAR: &str = "ZETCH_IN_TASK";

pub fn parent_task_active() -> bool {
std::env::var(IN_TASK_ENV_VAR).is_ok()
@@ -29,7 +29,7 @@ impl Task {
/// Run the task, post tasks will be given the env var to the post ctx path.
fn run(&self, config_filepath: &Path, cached_config_loc: Option<&Path>) -> Result<(), Zerr> {
// Make sure no recursion:
if std::env::var(IN_TASK_ENV_VAR).is_ok() {
if parent_task_active() {
return Err(zerr!(
Zerr::TaskRecursionError,
"Tasks being run recursively. Make sure you're not running a zetch command that triggers tasks from inside tasks.\nE.g. 'zetch render'.\n\nHint: 'zetch render|var' commands in 'pre' tasks won't work with due to recursive task constraints,\n however, 'zetch var' does work in 'post' tasks thanks to some internal magic."
8 changes: 0 additions & 8 deletions py_rust/src/config/validate.rs
Original file line number Diff line number Diff line change
@@ -137,14 +137,6 @@ fn format_err(err: Box<dyn valico::common::error::ValicoError>) -> String {
} else {
err.get_title()
};
info!(
"title: {}, detail {:?}, path: {}, code: {}, source: {:?}",
err.get_title(),
err.get_detail(),
err.get_path(),
err.get_code(),
err.source()
);

let mut loc_parts = err
.get_path()
8 changes: 8 additions & 0 deletions py_rust/src/state/active_state.rs
Original file line number Diff line number Diff line change
@@ -4,6 +4,8 @@ use std::{
};

use bitbazaar::timeit;
use parking_lot::Mutex;
use tempfile::NamedTempFile;

use super::parent_state::load_parent_state;
use crate::{args::Command, config::conf::Config, prelude::*};
@@ -25,6 +27,10 @@ pub struct State {

/// True if --superlight
pub superlight: bool,

// Storing the cached state file to prevent being dropped too early:
// Bit of a hack, but mutex easier than making state mutable where this needs to be set:
pub cached_state_file: Mutex<Option<NamedTempFile>>,
}

impl State {
@@ -42,6 +48,7 @@ impl State {
final_config_path: parent_shared_state.final_config_path,
light: false,
superlight: false,
cached_state_file: Mutex::new(None),
}
} else {
let final_config_path = build_final_config_path(
@@ -88,6 +95,7 @@ impl State {
final_config_path,
light,
superlight,
cached_state_file: Mutex::new(None),
}
};

16 changes: 12 additions & 4 deletions py_rust/src/state/parent_state.rs
Original file line number Diff line number Diff line change
@@ -8,7 +8,10 @@ use tempfile::NamedTempFile;

use super::State;
use crate::{
config::{conf::Config, tasks::parent_task_active},
config::{
conf::Config,
tasks::{parent_task_active, IN_TASK_ENV_VAR},
},
prelude::*,
};

@@ -26,15 +29,18 @@ pub struct StoredParentState {
///
/// Returns the PathBuf to the temporary file.
pub fn store_parent_state(state: &State) -> Result<PathBuf, Zerr> {
let state = StoredParentState {
let stored_state = StoredParentState {
conf: state.conf.clone(),
ctx: state.ctx.clone(),
final_config_path: state.final_config_path.clone(),
};

let temp = NamedTempFile::new().change_context(Zerr::InternalError)?;
serde_json::to_writer(&temp, &state).change_context(Zerr::InternalError)?;
Ok(temp.path().to_path_buf())
serde_json::to_writer(&temp, &stored_state).change_context(Zerr::InternalError)?;
let buf = temp.path().to_path_buf();
// Keep stored to prevent dropping and getting cleaned up too early:
state.cached_state_file.lock().replace(temp);
Ok(buf)
}

/// Load the cached state if it's available, return None otherwise.
@@ -51,6 +57,8 @@ pub fn load_parent_state() -> Result<Option<StoredParentState>, Zerr> {
return Ok(Some(
serde_json::from_str(&contents).change_context(Zerr::InternalError)?,
));
} else {
warn!("Nested zetch task seems to be running, tried loading parent state from {}, but it doesn't exist. You may have orphaned {}/{} environment variables.", path.display(), IN_TASK_ENV_VAR, CACHED_STATE_ENV_VAR);
}
}
Ok(None)
62 changes: 62 additions & 0 deletions py_rust/tests/test_tasks.py
Original file line number Diff line number Diff line change
@@ -149,3 +149,65 @@ def test_tasks_invalid(
)
with pytest.raises(ValueError, match=err_expected):
cli.render(manager.root_dir, conf_file)


@pytest.mark.parametrize(
"desc, config, cb",
[
# Important task! Written to catch bug.
# Make sure var works in post, and still everything works when pre exists too. (pre should work with read/put/del but not var):
(
"basic_no_cmd",
{
"tasks": {
"pre": [
{
"commands": [
# TODO after next bb update, replace the below with the commented out:
"echo foo",
# Random stuff, just not var as that wouldn't work in pre:
# 'echo \'{"ree": "randomo"}\' > file.json',
# "zetch read file.json ree",
# "zetch put file.json value bar",
# "zetch del file.json ree",
]
}
],
"post": [
{
"commands": [
'echo \'{"ree": "randomo"}\' > file.json',
"zetch read file.json ree",
"zetch put file.json value bar",
"zetch del file.json ree",
"zetch var FOO",
]
}
],
},
"context": {"static": {"FOO": {"value": "bar"}}},
},
lambda man: lambda: check_file(
os.path.join(man.root_dir, "file.json"), '{\n "value": "bar"\n}'
),
),
],
)
def test_tasks_various(
desc: str,
config: InputConfig,
# Setup callable returns either None or a callable that will be called after the test to run any other checks.
cb: "tp.Optional[tp.Callable[[TmpFileManager], tp.Optional[tp.Callable[[], tp.Any]]]]",
):
with TmpFileManager() as manager:
conf_file = manager.tmpfile(
zetch._toml_create(config),
full_name="zetch.config.toml",
)

post_check = cb(manager) if cb else None

cli.render(manager.root_dir, conf_file)

if post_check:
post_check()

0 comments on commit 53103e4

Please sign in to comment.