feat: support creating resources from spec#239
Conversation
Greptile SummaryAdded declarative spec-based resource creation using JSON files with create-or-reconfigure semantics. Implemented via new Key changes:
The implementation follows PUT semantics consistently across both CLI and Lite server paths. Spec files use JSON with humantime durations and kebab-case enums for user-friendliness. Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User: s2 apply -f spec.json] --> B[CLI: apply.rs]
B --> C{Parse & Validate Spec}
C -->|Invalid| D[Return Error]
C -->|Valid| E[SDK: create_or_reconfigure_basin]
E --> F[API: PUT /v1/basins/:name]
F --> G{Basin Exists?}
G -->|No| H[Create with Config]
G -->|Yes| I[Reconfigure Existing]
H --> J[Return 201 Created]
I --> K[Return 200 OK]
J --> L[CLI: Display Green]
K --> M[CLI: Display Yellow]
E --> N[SDK: create_or_reconfigure_stream]
N --> O[API: PUT /v1/streams/:name]
O --> P{Stream Exists?}
P -->|No| Q[Create with Config]
P -->|Yes| R[Reconfigure Existing]
Q --> S[Return 201 Created]
R --> T[Return 200 OK]
U[S2 Lite: --init-file spec.json] --> V[lite/init.rs]
V --> W[Backend: create_basin]
W --> X[CreateMode::CreateOrReconfigure]
V --> Y[Backend: create_stream]
Y --> Z[CreateMode::CreateOrReconfigure]
Last reviewed commit: cf69094 |
lite/src/init.rs
Outdated
| impl From<BasinConfigSpec> for BasinConfig { | ||
| fn from(s: BasinConfigSpec) -> Self { | ||
| BasinConfig { | ||
| default_stream_config: s | ||
| .default_stream_config | ||
| .map(OptionalStreamConfig::from) | ||
| .unwrap_or_default(), | ||
| create_stream_on_append: s.create_stream_on_append.unwrap_or(false), | ||
| create_stream_on_read: s.create_stream_on_read.unwrap_or(false), | ||
| } |
There was a problem hiding this comment.
Partial config silently resets unspecified fields
When CreateMode::CreateOrReconfigure is used (i.e., config is Some), the backend does a full replacement of the BasinConfig. Because this From impl defaults create_stream_on_append and create_stream_on_read to false when not specified in the spec, a user who provides a partial config (e.g., only default_stream_config) will unintentionally reset previously-set create_stream_on_append: true back to false.
This is consistent with PUT semantics, but may surprise users who expect partial-update behavior from a spec file. Consider documenting this full-replacement behavior prominently (e.g., in the --init-file help text or the spec format docs) so users understand that omitted fields revert to defaults on reconfigure.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: lite/src/init.rs
Line: 167-176
Comment:
**Partial config silently resets unspecified fields**
When `CreateMode::CreateOrReconfigure` is used (i.e., `config` is `Some`), the backend does a full replacement of the `BasinConfig`. Because this `From` impl defaults `create_stream_on_append` and `create_stream_on_read` to `false` when not specified in the spec, a user who provides a partial config (e.g., only `default_stream_config`) will unintentionally reset previously-set `create_stream_on_append: true` back to `false`.
This is consistent with PUT semantics, but may surprise users who expect partial-update behavior from a spec file. Consider documenting this full-replacement behavior prominently (e.g., in the `--init-file` help text or the spec format docs) so users understand that omitted fields revert to defaults on reconfigure.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
cli/src/apply.rs
Outdated
| let sdk_config = config | ||
| .as_ref() | ||
| .cloned() | ||
| .map(basin_config_to_sdk) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Unnecessary reconfigure call when config is None
When config is None, the "already exists" branch at line 184 calls config.map(basin_config_to_reconfig).unwrap_or_default(), resulting in a no-op reconfigure API call. The Lite path avoids this by using CreateMode::CreateOnly when there's no config. Consider short-circuiting here as well to skip the redundant network round-trip:
| let sdk_config = config | |
| .as_ref() | |
| .cloned() | |
| .map(basin_config_to_sdk) | |
| .unwrap_or_default(); | |
| let sdk_config = config | |
| .as_ref() | |
| .cloned() | |
| .map(basin_config_to_sdk) | |
| .unwrap_or_default(); | |
| let input = CreateBasinInput::new(basin.clone()).with_config(sdk_config); | |
| match s2.create_basin(input).await { | |
| Ok(_) => { | |
| eprintln!("{}", format!(" basin {basin}").green().bold()); | |
| } | |
| Err(ref e) if is_already_exists(e) => { | |
| if let Some(config) = config { | |
| let reconfig = basin_config_to_reconfig(config); | |
| s2.reconfigure_basin(ReconfigureBasinInput::new(basin.clone(), reconfig)) | |
| .await | |
| .map_err(|e| { | |
| miette::miette!("failed to reconfigure basin {:?}: {}", basin.as_ref(), e) | |
| })?; | |
| eprintln!( | |
| "{}", | |
| format!(" basin {basin} (reconfigured)").yellow().bold() | |
| ); | |
| } else { | |
| eprintln!( | |
| "{}", | |
| format!(" basin {basin} (already exists)").yellow().bold() | |
| ); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/apply.rs
Line: 172-176
Comment:
**Unnecessary reconfigure call when config is None**
When `config` is `None`, the "already exists" branch at line 184 calls `config.map(basin_config_to_reconfig).unwrap_or_default()`, resulting in a no-op reconfigure API call. The Lite path avoids this by using `CreateMode::CreateOnly` when there's no config. Consider short-circuiting here as well to skip the redundant network round-trip:
```suggestion
let sdk_config = config
.as_ref()
.cloned()
.map(basin_config_to_sdk)
.unwrap_or_default();
let input = CreateBasinInput::new(basin.clone()).with_config(sdk_config);
match s2.create_basin(input).await {
Ok(_) => {
eprintln!("{}", format!(" basin {basin}").green().bold());
}
Err(ref e) if is_already_exists(e) => {
if let Some(config) = config {
let reconfig = basin_config_to_reconfig(config);
s2.reconfigure_basin(ReconfigureBasinInput::new(basin.clone(), reconfig))
.await
.map_err(|e| {
miette::miette!("failed to reconfigure basin {:?}: {}", basin.as_ref(), e)
})?;
eprintln!(
"{}",
format!(" basin {basin} (reconfigured)").yellow().bold()
);
} else {
eprintln!(
"{}",
format!(" basin {basin} (already exists)").yellow().bold()
);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
cli/src/apply.rs
Outdated
| ) -> miette::Result<()> { | ||
| let sdk_config = config | ||
| .as_ref() | ||
| .cloned() | ||
| .map(stream_config_to_sdk) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Same unnecessary reconfigure for streams when config is None
Same pattern as apply_basin — when config is None and the stream already exists, line 225 issues a no-op reconfigure API call. Skipping it when config.is_none() avoids a redundant round-trip and gives the user a more accurate "(already exists)" message instead of "(reconfigured)".
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/apply.rs
Line: 211-216
Comment:
**Same unnecessary reconfigure for streams when config is None**
Same pattern as `apply_basin` — when `config` is `None` and the stream already exists, line 225 issues a no-op reconfigure API call. Skipping it when `config.is_none()` avoids a redundant round-trip and gives the user a more accurate "(already exists)" message instead of "(reconfigured)".
How can I resolve this? If you propose a fix, please make it concise.|
@greptile-apps review |
## 🤖 New release * `s2-api`: 0.27.6 -> 0.27.7 (✓ API compatible changes) * `s2-lite`: 0.29.7 -> 0.29.8 (✓ API compatible changes) * `s2-sdk`: 0.24.2 -> 0.24.3 (✓ API compatible changes) * `s2-cli`: 0.29.7 -> 0.29.8 <details><summary><i><b>Changelog</b></i></summary><p> ## `s2-api` <blockquote> ## [0.27.7] - 2026-02-24 ### Features - Support creating resources from spec ([#239](#239)) ### Documentation - Display default values for `create_stream_on_*` config ([#241](#241)) <!-- generated by git-cliff --> </blockquote> ## `s2-lite` <blockquote> ## [0.29.8] - 2026-02-24 ### Features - Support creating resources from spec ([#239](#239)) <!-- generated by git-cliff --> </blockquote> ## `s2-sdk` <blockquote> ## [0.24.3] - 2026-02-24 ### Features - Support creating resources from spec ([#239](#239)) <!-- generated by git-cliff --> </blockquote> ## `s2-cli` <blockquote> ## [0.29.8] - 2026-02-24 ### Features - Support creating resources from spec ([#239](#239)) <!-- generated by git-cliff --> </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). Co-authored-by: s2-release-plz[bot] <262023388+s2-release-plz[bot]@users.noreply.github.com>
credits: sonnet 4.6, codex 5.3