Skip to content

Add new ssh key argument and don't check magic number in old versions - #234

Closed
pablogsal wants to merge 4 commits into
python:mainfrom
pablogsal:new
Closed

Add new ssh key argument and don't check magic number in old versions#234
pablogsal wants to merge 4 commits into
python:mainfrom
pablogsal:new

Conversation

@pablogsal

Copy link
Copy Markdown
Member

No description provided.

@hugovk

Copy link
Copy Markdown
Member

(I pushed a test and lint fix)

Comment threadrun_release.py

def check_magic_number(db: ReleaseShelf) -> None:
release_tag = db["release"]
if release_tag.major == 3 and release_tag.minor <= 13:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to avoid hardcoding this, as it's another thing that needs adding to a longish list when a new version comes around.

Perhaps we can fetch the newest version from https://github.com/python/devguide/blob/main/include/release-cycle.json (and later from python/peps#4331)?

This can also be a followup PR.

@pablogsalpablogsalApr 8, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this is the first version that needed this feature. How fetching the newer version would be cleaner ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it always only the current feature release that this check will apply for? If so, perhaps expanding the security Boolean to a branch status enum would be better, as then we could test if branch-status == feature here.

A

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No, this particular check applies to all releases that are after we changed that file, that is, anything above or including 3.13

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, if the limit is always 3.13 and we don't need to change it, then a hardcoded version makes sense here.

Let's use a tuple so we don't need to worry about Python 4 :)

Suggested change
ifrelease_tag.major==3andrelease_tag.minor<=13:
ifnotrelease_tag.as_tuple() >= (3, 14):

@merwokmerwokNov 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this be written using <= ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, or rather as < (3, 13). I'll open a new PR.

Comment threadrun_release.py
Comment on lines +1310 to +1316
parser.add_argument(
"--security-release",
dest="security_release",
action="store_true",
default=False,
help="Indicate this is a security release (only checks for Linux files)",
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And similarly, we could check the status in the JSON file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here's a PR to check the status from the devguide, so the RM doesn't have to remember to pass --security each time:

pablogsal#1

Merging that will update this PR.

@hugovkhugovk mentioned this pull request Sep 11, 2025
@hugovk

Copy link
Copy Markdown
Member

I've split out the --ssh-key addition into #279.

@hugovk

Copy link
Copy Markdown
Member

@hugovk

Copy link
Copy Markdown
Member

All merged, thanks all!

@hugovkhugovk closed this Nov 12, 2025
Sign up for freeto 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.

4 participants

@pablogsal@hugovk@merwok@AA-Turner