-
Notifications
You must be signed in to change notification settings - Fork 4
[ENG-2326] "Now" is translated to None value as time for reserved nodes in Nodes API #215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENG-2326] "Now" is translated to None value as time for reserved nodes in Nodes API #215
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
Fixed redundant code in reserved node creation logic that was incorrectly setting start_at when user provided "NOW" as start time, and updated TypeScript schema to properly reflect that start_at can be number | null.
Key changes:
- Removed lines 259-260 in
create.tsthat setstart_atin the else clause, since line 279-284 unconditionally override this value anyway - Updated schema type for
start_atfromnumbertonumber | nullto align with API requirements - The logic now correctly handles "NOW" start times: validation passes at line 261, and final timestamp is set at lines 279-284
Confidence Score: 5/5
- This PR is safe to merge with no risk
- The changes remove redundant code that was being overridden anyway, and update the schema to match API requirements. The logic flow remains correct: when
start === "NOW", the validation passes and the final timestamp is properly set at lines 279-284. No behavioral changes, just cleanup. - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/lib/nodes/create.ts | 5/5 | Removed redundant code that was setting start_at for "NOW" case since it was being overridden by lines 279-284 anyway |
| src/schema.ts | 5/5 | Updated TypeScript schema to allow start_at to be `number |
Sequence Diagram
sequenceDiagram
participant User
participant CLI
participant Parser as parseStartDateOrNow
participant Validation
participant API as Nodes API
User->>CLI: sf nodes create --start NOW --duration 1h
CLI->>Parser: parseStartDateOrNow("NOW")
Parser-->>CLI: "NOW" (string)
Note over CLI: createParams.start_at not set<br/>(lines 256-258 skipped)
CLI->>Validation: Check if "NOW" is valid
Validation-->>CLI: Valid (startDate === "NOW")
Note over CLI: Lines 279-284: Set final start_at
CLI->>CLI: start_at = Math.floor(new Date().getTime() / 1000)
CLI->>API: POST /nodes/create {start_at: <timestamp>, ...}
API-->>CLI: Node created
CLI-->>User: Success
2 files reviewed, no comments
0xJepsen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a code owner here but these changes look fairly constrained and correct to me
sigmachirality
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small issues
- you did not regenerate the Stainless SDK.
- you directly edited the schema.ts instead of running deno task schema
- the schema.ts does not affect the Stainless SDK.
big issues:
- due to how the logic interacts with existing date hour rounding logic, this 1 line change doesn’t actually work.
|
Yikes ok. |
|
If i revert this PR can I bribe you into doing it for me? Either that or I need an education here - will be stopping by desk to ask as well |

We are running into a race condition where confirmation steps lead to time now being past 1 min