Conversation
A code app's endpoint path is its handler's method name with underscores turned into hyphens, so renaming a method moves a public endpoint and 404s its callers with nothing on the path saying so. An update run with --wait now reads the endpoint set before the deploy and again once the rollout lands, and reports what moved. Only on that combination: a create has no previous set to compare against, and without --wait the command returns before the build that decides the new one. The report goes to stderr so --format json stays machine-readable on stdout, and a failed read costs the warning rather than the deploy. RUNSERV-763 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading the app's endpoint set before the deploy can fail, and carrying on with an empty reading made every endpoint the app already served look newly added. A deploy that changed nothing would then print a warning naming the whole set. A warning that cries wolf on an unchanged deploy is worse than no warning: the one it exists to raise is a real rename. So a failed before-read now suppresses the report entirely rather than reporting against nothing. RUNSERV-763 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard was two conditions inline with the reasoning in a comment. Naming it puts the reasoning on the predicate and makes both halves testable: with no reading from before the deploy the report would call the app's whole existing set new, and before the rollout activates it would compare the old set against itself. RUNSERV-763 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. To trigger a review, include ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Endpoint retrieval must handle cursor pagination to avoid inaccurate reports for large endpoint sets.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Adds deploy-time reporting for endpoint additions and removals on waited updates.
Changes:
- Captures endpoint sets before and after eligible deployments.
- Reports endpoint changes and potential 404s to stderr.
- Adds endpoint comparison, reporting, lifecycle, and API-read tests.
- Documents the new behavior.
| File | Summary |
|---|---|
internal/cmd/serverless/display_test.go |
Reuses endpoint test constants. |
internal/cmd/serverless/deploy.go |
Integrates endpoint comparison into deploy flow. |
internal/cmd/serverless/deploy_endpoints.go |
Implements endpoint retrieval, diffing, and reporting. |
internal/cmd/serverless/deploy_endpoints_test.go |
Tests endpoint comparison and reporting. |
docs/runware_serverless_deploy.md |
Documents endpoint-change reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The file explained itself three times over: what an endpoint path is, which the deploy help already says, and a paragraph per helper restating its own name. Kept only what the code cannot say — why the report goes to stderr, why both guard conditions exist, and why one page is the whole set. Comment-only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reader took one page and my comment justified it with a page size of 100. The shared limit parameter defaults to 20, and an app may declare 20 endpoints, so a full app already sits exactly on the page boundary. A short read is not a missing warning but a false one: the endpoints it never saw look removed, and the report tells the customer their callers are about to 404 on paths that never moved. It now follows the cursor, asking for the contract maximum so the common app still takes one round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
danmrichards
requested changes
Sep 21, 2026
…oints A source update on an app that is already active answers `active` while its new build runs, so AppDeployTerminal was true immediately, --wait skipped polling, and the endpoint rows read back were still the outgoing version's. Every rename on an existing app therefore compared a set against itself and reported nothing -- the one case this feature is for. The pin is what moves when the submitted version activates, so the comparison now waits on activeVersionId rather than on status. It gives up when no activation can arrive: the app left the states a roll lands in, or the build failed, which on a live app leaves the status active and the pin where it was and would otherwise poll forever. shouldReportEndpointChange goes with it. The pin move proves what its status check approximated, and proves it for the case the status check got wrong. Also make the 404 line source-neutral: container apps take their paths from container.yaml, not from a handler name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address snapshot comparison, superseded-build handling, and submitted-build identification before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
The endpoint set and the active version were read independently, so a failed app read left the pin nil while the comparison still ran. A nil pin is indistinguishable from an app that has never activated a version, which activationMoved counts as a move -- so the wait returned on its first poll, against the outgoing endpoint set, reintroducing exactly the silent miss the wait was added to fix. Both reads now succeed together or the comparison is skipped, which is the same rule the before-read already followed: a failed read costs the report, never turns it into a false one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A code app's endpoint path is its handler's method name with underscores hyphenated, so renaming a method moves a public endpoint and 404s its callers.
deploynow reads the endpoint set before the deploy and again once the rollout lands, and reports what moved. A report, not a gate.Only on an update run with
--wait: a create has no previous set, and without--waitthe command returns before the build that decides the new one. A failed read of the set before the deploy suppresses the report rather than comparing against nothing, which would make the app's whole set look new.Computed client-side, so it needs no API field and is independent of the API side. Written to stderr.