Fix renameTable to return 204 instead of 200#4383
Conversation
|
While I think this change is correct (and is correcting an error), I believe all API changes need a VOTE thread? @dimas-b thoughts? |
Isn't this also a possibly breaking change? Although the current implementation clearly doesn't conform with IRC spec, we change here the actual response status here. |
There was a problem hiding this comment.
I think the fix is correct. Please consider asserting the status in AbstractIcebergCatalogTest or an *IT.java so this doesn't silently regress.
Optional: one-line CHANGELOG entry under Fixed since this is an externally observable HTTP status change.
|
Technically this is a breaking change at the API level, IMHO. However, given that Polaris implements the spec incorrectly in this case and the Iceberg spec change that altered this response code predates Polaris, I think that the fix is good. Thanks for noticing it, @viditochani ! We should definitely discuss it on I do not think we need a formal |
|
@viditochani : Could you add a note about this to |
viditochani
force-pushed
the
fix-rename-status-code
branch
from
a036548 to
96b27b6
Compare
viditochani
force-pushed
the
fix-rename-status-code
branch
from
96b27b6 to
1842953
Compare
The rename table endpoint returns 200 with a text body instead of the 204 (No Content) status as per the IRC spec.
viditochani
force-pushed
the
fix-rename-status-code
branch
from
1842953 to
f30440e
Compare
|
Looks like the Zizmor security audit is failing with three warnings about action hash pins missing/mismatched version comments at commit 68bde559dea0. I'd suggest to rebase against the latest main branch. |
The skill's worked example showed both the create-pending-review call and a follow-up `gh api .../events -f event=COMMENT` submit call. The model followed the example end-to-end and submitted reviews directly, bypassing human review (e.g. apache/polaris#4383 review 4245835033 was posted as COMMENTED instead of pending). Drop the submit step from the example, remove the parenthetical that listed submit-immediately events as a menu option, and explicitly forbid calling `/events` or passing an `event` field. Pending with no `event` is now stated as the only acceptable end state.
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
github-project-automation
Bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
|
Retriggered the CIs. |
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
+1 Thanks @viditochani for the fix!
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
Bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
Thanks @viditochani for the fix! Thanks everyone for the review. |
5 participants
renameTablereturns HTTP 200 with body text"NO_CONTENT"instead of HTTP 204 as per the IRC spec.Testing
Manually verified by sending a rename table request to a local Polaris instance.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)