Skip to content

PYTHON-6132 Re-sync specs PR improvements - #3082

Merged
blink1073 merged 13 commits into
mongodb:mainfrom
blink1073:PYTHON-6132
Sep 30, 2026
Merged

blink1073 merged 13 commits into
mongodb:mainfrom
blink1073:PYTHON-6132

Conversation

@blink1073

@blink1073 blink1073 commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

PYTHON-6132

Changes in this PR

  • cp_psl runs under a Python 3.7+ interpreter and converts the Public Suffix List atomically, so failures can no longer ship an emptied pymongo/public_suffix_list.dat.
  • Failed syncs are now logged to the task log, and PR bodies are written as plain text (fixes malformed bodies).
  • PR creation improvements: same-day reruns are idempotent: force-push the date-stamped branch, refresh the existing PR instead of duplicating it, attribute commits to the PR bot, and retry the push on transient 403s.

Test Plan

  • Pre-commit hooks and bash -n pass; cp_psl verified against the real specs-repo list (10248 lines in/out, 459 Punycode conversions; failure cases leave the destination untouched).
  • Tested end-to-end on Evergreen: branch pushed and PR [Spec Resync] 09-30-2026 #3081 created by the bot.

Checklist

Checklist for Author

  • Did you update the changelog (if necessary)? — N/A
  • Is there test coverage? - N/A - ran manual patch
  • Is any followup work tracked in a JIRA ticket? If so, add link(s). N/A

Checklist for Reviewer

  • Does the title of the PR reference a JIRA Ticket?
  • Do you fully understand the implementation? (Would you be comfortable explaining how this code works to someone else?)
  • Is all relevant documentation (README or docstring) updated?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The existing-PR update can falsely report success after HTTP errors, and the final push failure incurs an unnecessary retry delay.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Hardens the automated specification-resync pipeline and PR creation workflow.

Changes:

  • Makes Public Suffix List conversion atomic.
  • Improves failure logging and PR-body encoding.
  • Adds bot attribution, push retries, and same-day PR reuse.
File Description
.evergreen/​resync-specs.sh Safely converts and replaces the suffix list.
.evergreen/​scripts/​resync-all-specs.sh Selects a suitable Python interpreter.
.evergreen/​scripts/​resync-all-specs.py Logs failures and writes plain-text summaries.
.evergreen/​scripts/​create-spec-pr.sh Improves automated branch pushes and PR updates.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .evergreen/scripts/create-spec-pr.sh Outdated
Comment thread .evergreen/scripts/create-spec-pr.sh Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (2)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Make reruns reuse or reset the local branch

.evergreen/​scripts/​create-spec-pr.sh:53

Force-pushing the date-stamped remote branch does not make reruns from the same checkout idempotent: the earlier git checkout -b $branch "origin/main" leaves that local branch in place, so a second invocation fails with branch ... already exists before reaching this retry loop. Reuse/reset the local branch (or use a clean worktree) before pushing.

Medium severity Fail or retry PR lookup on HTTP errors

.evergreen/​scripts/​create-spec-pr.sh:82

The PR lookup does not fail on HTTP errors. A transient 403/5xx produces an error object, but because this curl lacks --fail and the pipeline has no pipefail, jq yields an empty URL and the script proceeds as though no PR exists; the subsequent POST can fail with an already-existing PR instead of refreshing it. Make this lookup fail (or retry) on HTTP/API errors before deciding to create a PR.

Low severity Capture task-log stderr as readable text

.evergreen/​scripts/​resync-all-specs.py:65

The newly added task-log message formats exc.stderr as a Python bytes object because this subprocess.run call does not set text=True; patch failures therefore appear as b'...\\n' with escaped newlines instead of readable stderr. Request text output here as is already done for the spec-sync subprocess.

Comment thread .evergreen/resync-specs.sh
Comment thread .evergreen/scripts/resync-all-specs.sh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues affect CI interpreter selection and rerun behavior.

Review effort: Lite
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Reuse existing branch for idempotent reruns

.evergreen/​scripts/​create-spec-pr.sh:35

The force-push does not make reruns idempotent when the Evergreen checkout is reused: git checkout -b $branch above fails if the date-stamped branch already exists locally, so execution never reaches this retry/force-push block or the existing-PR refresh. Reset or reuse the branch (for example with git checkout -B "$branch" "origin/main") before committing.

Low severity Capture subprocess stderr as text

.evergreen/​scripts/​resync-all-specs.py:65

apply_patches captures stderr without text=True, so exc.stderr is bytes here. The new task-log message (and the PR summary built from the same value) will therefore contain the escaped b'...' representation instead of the actual diagnostic text. Capture text output in the subprocess call so both consumers receive a normal string.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Address the two moderate issues in create-spec-pr.sh involving worktree cleanup and API lookup failure handling.

Review effort: Lite
Findings: None

@blink1073
blink1073 marked this pull request as ready for review September 30, 2026 13:54
@blink1073
blink1073 requested a review from a team as a code owner September 30, 2026 13:54
@codecov

codecov Bot commented Sep 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sleepyStick sleepyStick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:0 love the improvements!

@blink1073
blink1073 merged commit 6cbedaf into mongodb:main Sep 30, 2026
90 of 92 checks passed
@blink1073
blink1073 deleted the PYTHON-6132 branch September 30, 2026 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants