change HTTP method to comply with spec #9100
Conversation
There is discrepancy with the spec, it has PUT
WalkthroughThe changes across the various files involve modifying the HTTP method for handling archival configuration requests from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
Client->>Server: PUT /v1/tenant/{tenant_id}/timeline/{timeline_id}/archival_config
Server-->>Client: 200 OK (Updated successfully)
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
storage_controller/src/pageserver_client.rs (1)
Server-side PUT method implementation missing
The change from POST to PUT for the
timeline_archival_configendpoint updates the HTTP method. However, the server-side implementation for handling PUT requests was not found. This may lead to the server not recognizing PUT requests for this endpoint, causing potential runtime errors and inconsistent API behavior.Please ensure that the server correctly handles PUT requests for
timeline_archival_configand update the API documentation to reflect this change.Analysis chain
Line range hint
239-246: Approve the HTTP method change and verify consistencyThe change from POST to PUT for the
timeline_archival_configendpoint aligns with the PR objectives to comply with the spec. This modification implies that the endpoint is now used for updating existing resources rather than creating new ones.To ensure consistency and prevent potential issues, please run the following verification steps:
Check for any client code that might be affected by this change:
Verify that the server-side implementation correctly handles PUT requests for this endpoint:
Update any relevant API documentation to reflect this change:
Please ensure that all affected areas are updated accordingly and that the change is thoroughly tested to prevent any regressions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any calls to timeline_archival_config in the codebase rg --type rust "timeline_archival_config" -C 5Length of output: 9025
Script:
#!/bin/bash # Search for the server-side implementation of the timeline_archival_config endpoint rg --type rust "fn.*timeline_archival_config.*Method::Put" -C 10Length of output: 66
Script:
#!/bin/bash # Search for API documentation mentioning timeline_archival_config rg --type markdown "timeline_archival_config"Length of output: 45
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- pageserver/client/src/mgmt_api.rs (1 hunks)
- pageserver/src/http/routes.rs (1 hunks)
- storage_controller/src/http.rs (1 hunks)
- storage_controller/src/pageserver_client.rs (1 hunks)
- test_runner/fixtures/pageserver/http.py (1 hunks)
Additional comments not posted (2)
test_runner/fixtures/pageserver/http.py (1)
634-636: Approve changing the HTTP method from POST to PUT.Modifying the HTTP method from POST to PUT for updating the archival configuration aligns with the RESTful convention of using PUT for updates to existing resources. As long as the server endpoint handles PUT requests correctly for this operation, this change improves the adherence to HTTP method semantics without altering the functionality.
storage_controller/src/http.rs (1)
Line range hint
1852-1862: HTTP method change aligns with RESTful principles, but consider implicationsThe change from POST to PUT for the
/v1/tenant/:tenant_id/timeline/:timeline_id/archival_configendpoint aligns better with RESTful API design principles, as PUT is typically used for updating existing resources. However, consider the following points:
Backward compatibility: This change might break existing clients expecting a POST endpoint. Ensure that this change is communicated clearly in the API documentation and consider versioning the API if backward compatibility is a concern.
Consistency: Verify that this change is consistent with other similar endpoints in the API to maintain a uniform design.
Implementation impact: Review the
handle_tenant_timeline_archival_configfunction to ensure it correctly handles the semantics of a PUT request (i.e., updating an existing resource) rather than a POST request (creating a new resource).To check for consistency and potential impacts, run the following script:
5003 tests run: 4839 passed, 0 failed, 164 skipped (full report)Flaky tests (3)Postgres 17
Postgres 16
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
7564667 at 2024-09-23T13:52:00.946Z :recycle: |
There is discrepancy with the spec, it has PUT
Problem
Summary of changes
Checklist before requesting a review
Checklist before merging
Summary by CodeRabbit
New Features
Bug Fixes