Repository navigation
feat(api): add OCI external-storage provider support - #4060
Conversation
|
Review follow-up before merge: the OCI response schema currently produces separate endpoint-local classes for configuration responses and external-storage audit events. AWS, Azure, and GCP use shared exported response models. Please assess the requested shared Offline checks passed for Python sync/async OCI create requests, configuration response decoding, and audit-log OCI discriminator selection. No runtime or authentication changes are included. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 50 mixed files remain; 0 existing customizations changed; 1 generated baselines changed. Compared
49 existing customizations unchanged
9 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 38180190669 --repo openai/openai-python \
--name castiron-custom-code-38180190669-1 --dir /tmp/castiron-custom-code-38180190669-1
git apply --stat /tmp/castiron-custom-code-38180190669-1/custom-code.patch
cat /tmp/castiron-custom-code-38180190669-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin b5702792a0adb1e86a8b51709e935c1e0acde727 2e469968e0f38b7d71df1c403f5ed0c839e9e297
python3 scripts/castiron/custom_code_report.py report \
--base b5702792a0adb1e86a8b51709e935c1e0acde727 \
--head 2e469968e0f38b7d71df1c403f5ed0c839e9e297 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-2e469968e0f3
cat /tmp/castiron-custom-code-2e469968e0f3/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cf249792d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed all 5 changed files at 2cf249792d64bf02fb2f412a64e0bae7bf70dbf0. Approval withheld for the response-model split.
The OCI configuration and audit-log response unions use two different Python classes for the same response schema. I independently confirmed the existing P2 finding: a handler typed for the configuration provider, or an isinstance check against its OCI class, cannot handle the OCI audit provider as it handles AWS, Azure, and GCP. Generate and export one shared OCI response model and use it in both unions; keep the request TypedDict separate. I have not duplicated the inline comment.
Request fields, discriminator values, sync/async transform paths, and existing providers otherwise match the schema.
I verified the full diff, immutable source hashes, merge base, and matching schema changes across all four SDK candidates. Exact-head CI reports 18 successful and 10 skipped checks, with no failing or pending checks reported. This was a source-only review; I did not run tests, builds, generation, or SDK execution.
yoni-oai
left a comment
yoni-oai
left a comment
There was a problem hiding this comment.
Reviewed the complete change; the existing OCI response-model compatibility finding remains actionable, so approval is withheld.
2cf2497 to
f2f2323
Compare
|
The previous external compatibility failure was before any SDK test: OkTest run 38172542979 could not retrieve public metadata for The test-only correction superseded the failed subject: current head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2f232340f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Castiron-Internal-PR: openai/openai-python-internal#237 Castiron-Source-SHA: 3f7192fdd149a176d1412b7dfe6febbeb273bb3f Castiron-Public-Base-SHA: b570279
f2f2323 to
2e46996
Compare
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed all nine changed files at 2e469968e0f38b7d71df1c403f5ed0c839e9e297. The earlier shared-model and audit-test findings are addressed. No actionable findings remain in this review.
Both response unions now use the same exported OciExternalStorageProvider; the request TypedDict remains separate. The committed synchronous and asynchronous transport tests cover OCI create serialization, strict configuration and audit-event parsing, concrete provider identity, admin authentication, and unknown-field round trips. Existing provider and pagination coverage remains intact.
I verified the complete diff and pinned source hashes. This was a source-only review; I did not execute tests, builds, or generation.
At inspection, the current head had 17 successful and 9 skipped checks. The current-head OkTest report also showed 236/236 SDK tests passing.
|
Post-merge CI diagnosis for main The failing assertion is the fixture cleanup's The fixture records connections only when its server handler begins; the client closes as soon as the iterable pauses, without a handler-entry barrier. In the pinned websockets 15.0.1 server, the handshake completes before the OPEN-state assertion and handler invocation. A close/handler-start scheduling race is consistent with the zero recorded connections. This is a supported hypothesis, not a proven exact interleaving. Evidence: the test and runtime blobs match pre-OCI main byte-for-byte ( Requested one retry of only the failed Python 3.10 job: https://github.com/openai/openai-python/actions/runs/38181680650/job/114595428732 . No code, assertions, timing thresholds, dependencies, credentials, or policies changed. If the failure recurs, stop for separate concurrency/test scope instead of retrying again. |
|
The single targeted retry passed on unchanged main commit |
3 participants
Summary
Adds OCI to external-storage provider requests and responses.
Changes