Skip to content

feat: modify command in SKEL (SK-167)#227

Merged
drmorr0 merged 1 commit intomainfrom
drmorr/skel-apply-cmd
Mar 17, 2026
Merged

feat: modify command in SKEL (SK-167)#227
drmorr0 merged 1 commit intomainfrom
drmorr/skel-apply-cmd

Conversation

@drmorr0
Copy link
Copy Markdown
Contributor

@drmorr0 drmorr0 commented Mar 11, 2026

Related Links

  • SK-167

Description and Rationale

  • modify is now a fully-supported SKEL function (originally we called this apply but I decided late in the process that modify was better)
  • Checked additional variable errors/issues in the AST generation instead of the engine
  • build module updated to support make debug-test
  • fix the rhs_to_value function in the event that the first component isn't a variable
  • updated the docs

How

Checking defined variables in AST generation

It turns out we were already tracking a defined_vars set in the ast code, so it was a "relatively" simple matter of plumbing that through to more places. It's a little subtle/tricky, because in the left-hand-side of a resource test, you can only reference the variable that's defined in that test, but in the right-hand side you can reference variables that were defined in previous tests. In other words, this is invalid:

$x := some.path | $y == foo

But this is valid:

$x := some.path | exists($x) && $y := some.other.path | $y == $x

We also got rid of the var_prefixed_resource_path rule in the grammar, because it turns out a) it over-complicated the grammar, and b) it disallowed some valid expressions.

Modify SKEL command

This was more-or-less straightforward, we just needed to ensure that all the different variants in the assignment worked correctly (e.g., path = val, $x.path = val, path = $y, and $x.path = $y).

We created a reify_pointers function which takes an abstract pointer and the variable context, and converts that to a list of concrete pointers that match the object with all the variables filled in. This actually simplifies the logic for the remove command as well, and fixes a couple corner cases I didn't think about before (specifically, if the variable substitution doesn't work because we didn't reference a particular variable in the abstract pointer, that would have silently gone through before; I don't think it actually would do anything but still probably isn't ideal).

Test Steps

  • New unit and integration tests written
  • manual testing against some real traces using a file like the following, confirmed that the node selectors were updated as expected
modify(@t >= 1h && spec.template.spec.nodeSelector."simkube.dev/foo" == "bar", spec.template.spec.nodeSelector."simkube.dev/foo" = "baz");

Other notes

When I was writing this before, I forgot about the klabel macro, which is super-handy. Would love to get some of this stuff extracted out into a shared library at some point.

  • I certify that this PR does not contain any code that has been generated with GitHub Copilot or any other AI-based code generation tool, in accordance with this project's policies.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 97.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.70%. Comparing base (f7b6f42) to head (7f22473).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
sk-cli/src/skel/ast.rs 97.56% 2 Missing ⚠️
sk-cli/src/skel/engine.rs 97.46% 2 Missing ⚠️
sk-cli/src/skel/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   78.23%   78.70%   +0.46%     
==========================================
  Files          63       63              
  Lines        3690     3789      +99     
==========================================
+ Hits         2887     2982      +95     
- Misses        803      807       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@drmorr0 drmorr0 force-pushed the drmorr/skel-apply-cmd branch 2 times, most recently from dd75036 to d078a3c Compare March 12, 2026 07:03
@drmorr0 drmorr0 changed the title feat: apply command in SKEL feat: apply command in SKEL (SK-167) Mar 12, 2026
@linear
Copy link
Copy Markdown

linear Bot commented Mar 12, 2026

@drmorr0 drmorr0 force-pushed the drmorr/skel-apply-cmd branch 4 times, most recently from dbbeda7 to 4415511 Compare March 17, 2026 21:00
@drmorr0 drmorr0 changed the title feat: apply command in SKEL (SK-167) feat: modify command in SKEL (SK-167) Mar 17, 2026
@drmorr0 drmorr0 force-pushed the drmorr/skel-apply-cmd branch from 4415511 to 7f22473 Compare March 17, 2026 21:01
@drmorr0 drmorr0 merged commit bb2380c into main Mar 17, 2026
7 checks passed
@drmorr0 drmorr0 deleted the drmorr/skel-apply-cmd branch March 17, 2026 21:18
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.

1 participant