add announcements subcommand to gen release announcement text #573

gotmax23 · 2024-01-25T03:49:23Z

No description provided.

webknjaz · 2024-01-25T13:15:35Z

src/antsibull/announcements.py

+async def verify_dists(dists: SdistAndWheel, dist_dir: Path) -> str | None:
+    for dist in dists:
+        file = dist_dir / dist.filename
+        if not await asyncio.to_thread(file.is_file):


Wow, I didn't realize you don't need parens with await in such cases..

Anyway, I think this is hard to parse. Would you consider labeling this check with a human-readable var?

Suggested change

if not await asyncio.to_thread(file.is_file):

dist_file_exists = await asyncio.to_thread(file.is_file)

if not dist_file_exists:

I think, it'd be good for readability.

I'm not a big fan of assigning an extra variable here. The reason I added the asyncio.to_thread thing is that file.is_file is a sync function that calls stat() which is technically blocking IO. I guess it doesn't really matter in this code (there's no concurrency going on here), so I could just call the sync method directly.

Async could persist, it's just that a named variable communicates the high-level intent of the check, which refers to the low-level details otherwise.

There's a difference between "an arbitrary file exists" and "a distribution is available". OTOH, I suppose this could be improved by renaming the file variable (which used to be a builtin, alias of open(), by the way).

src/antsibull/pypi.py

webknjaz · 2024-01-25T13:41:24Z

src/antsibull/pypi.py

+        """
+        checks: list[bool] = [
+            await verify_hash(file, self.sha256sum),
+            file.name == self.filename,


It would be much cheaper to make this a separate check and bail early, before attempting hashing. It's already performed unconditionally regardless of the hash verification result, anyway.

That's true. I guess I'll have to get rid of the list of checks thing. I wish there were async lambdas so I could do a list of those instead.

A had some utils for async mapping and stuff somewhere in octomachinery. Plus there was some utility lib for func/itertools like async stuff. But that's an overkill here.
Also, frameworks like Trio have better abstractions for this. And some of them inspired stdlib improvements, but I don't think many would be possible to adopt there. Anyway, I like running everything through the anyio shim layer to get the best of two worlds...

gotmax23

Thanks for the feedback, @webknjaz! I will apply it later.

src/antsibull/pypi.py

gotmax23 · 2024-01-25T21:25:08Z

src/antsibull/pypi.py

+        """
+        checks: list[bool] = [
+            await verify_hash(file, self.sha256sum),
+            file.name == self.filename,


That's true. I guess I'll have to get rid of the list of checks thing. I wish there were async lambdas so I could do a list of those instead.

gotmax23 · 2024-01-25T21:27:37Z

src/antsibull/announcements.py

+async def verify_dists(dists: SdistAndWheel, dist_dir: Path) -> str | None:
+    for dist in dists:
+        file = dist_dir / dist.filename
+        if not await asyncio.to_thread(file.is_file):


I'm not a big fan of assigning an extra variable here. The reason I added the asyncio.to_thread thing is that file.is_file is a sync function that calls stat() which is technically blocking IO. I guess it doesn't really matter in this code (there's no concurrency going on here), so I could just call the sync method directly.

gotmax23 · 2024-01-26T03:48:30Z

@webknjaz, I amended the commits with your feedback and gave you a Co-authored-by:. Thanks!

gotmax23 · 2024-01-26T07:07:55Z

The lint session failures are fixed by #574

gotmax23 · 2024-01-26T07:20:54Z

Rebased on top of main

src/antsibull/announcements.py

src/antsibull/cli/antsibull_build.py

gotmax23 · 2024-01-26T18:58:01Z

The lint session failures are fixed by #574

Well, the formatter ones were... There were still pylint issues.

gotmax23 · 2024-02-01T03:31:15Z

I'm marking this as ready for review. I think we can address the yarl transition separately.

webknjaz · 2024-02-01T13:07:04Z

Okay, fair. I don't see any obvious problems, otherwise, except for maybe --user that I remember bringing up elsewhere at some point. User-global installs are almost as harmful as the interpreter-global ones. But I suppose you didn't want to change the templates here, which is fine for now. Still, I'd link a PyPUG doc on installing apps for people to explore this.

felixfontein · 2024-02-05T22:04:07Z

pyproject.toml

+    # pydantic already pulls it in, but we use it for TypedDict
+    "typing_extensions",
+    # Used for sending announcements
+    "pyperclip",


This module doesn't seem to be maintained anymore (see for example asweigart/pyperclip#248, the referenced issues, and the absence of any comments of the author in these issues). I don't like the idea of adding a dependency to something that's already dead.

Also it might make sense to make this dependency, or alternatives like clipman, optional, so that they only need to be installed if they're actually used.

Ah, that's too bad about pyperclip. I'm still a bit partial to pyperclip, as I've used it a while, it's stable, and, unlike clipman, it has at least some basic unit tests. I can try out clipman and make it an optional dependency if you prefer, though.

Meh, clipman fails with

Failed to setup the clipboard engine: Please install dbus-python package. - Via your system package manager. Possible package names: "python3-dbus-python" or "python3-dbus" - Or via PIP: "pip3 install dbus".

for me on KDE Plasma. It should fall back to xclip or wl-clipboard like pyperclip does, but instead it's failing, because it doesn't have the dbus module to communicate with klipper.

For now, I think I'll keep pyperclip and make it optional. We can always switch away from it or add a fallback if pyperclip starts causing problems.

And yes, I know I can install the dbus-python module, but that requires installing dbus development headers and compiling the wheel locally or not using system packages instead of a venv.

For now, I think I'll keep pyperclip and make it optional. We can always switch away from it or add a fallback if pyperclip starts causing problems.

This was done in 28fb8de. Let me know what you think of that solution.

Looks great, thanks! It's annoying that the state of Python clipboard libraries seems to be somewhat a mess :)

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>

Add a new send-announcements process to interactively send announcements to email, the forum, and Matrix. Instead of requiring release managers to generate API tokens and input passwords, I chose this compromise approach. The command automatically opens up pre-filled email and forum posts and prompts the user about posting to Matrix. Credits to the Ansible Core developers whose release script that uses the webbrowser module in a similar way inspired this approach.

This makes it easier to chain together the announcements and send-announcements command.

pyproject.toml

felixfontein · 2024-02-07T06:47:38Z

@gotmax23 thanks a lot for implementing this!
@webknjaz thanks a lot for reviewing!

gotmax23 added the enhancement New feature or request label Jan 25, 2024

gotmax23 force-pushed the announcement branch from 0cb95cc to d480ed8 Compare January 25, 2024 03:51

gotmax23 mentioned this pull request Jan 25, 2024

Create a GitHub Actions to draft the release announcements ansible-community/ansible-build-data#351

Open

webknjaz reviewed Jan 25, 2024

View reviewed changes

src/antsibull/pypi.py Outdated Show resolved Hide resolved

webknjaz reviewed Jan 25, 2024

View reviewed changes

src/antsibull/pypi.py Show resolved Hide resolved

webknjaz reviewed Jan 25, 2024

View reviewed changes

gotmax23 commented Jan 25, 2024

View reviewed changes

gotmax23 force-pushed the announcement branch from d480ed8 to 1b9e881 Compare January 26, 2024 03:47

gotmax23 force-pushed the announcement branch 2 times, most recently from f568b44 to a6c292e Compare January 26, 2024 07:05

gotmax23 force-pushed the announcement branch from a6c292e to 3de0a78 Compare January 26, 2024 07:20

webknjaz reviewed Jan 26, 2024

View reviewed changes

src/antsibull/announcements.py Show resolved Hide resolved

webknjaz reviewed Jan 26, 2024

View reviewed changes

src/antsibull/announcements.py Outdated Show resolved Hide resolved

webknjaz reviewed Jan 26, 2024

View reviewed changes

src/antsibull/cli/antsibull_build.py Show resolved Hide resolved

gotmax23 force-pushed the announcement branch 2 times, most recently from 2b39cab to 837316b Compare January 26, 2024 21:03

gotmax23 changed the title ~~DRAFT: add announcements subcommand to gen release announcement text~~ add announcements subcommand to gen release announcement text Feb 1, 2024

gotmax23 marked this pull request as ready for review February 1, 2024 03:30

felixfontein mentioned this pull request Feb 5, 2024

Adds directories & email announcement for minor release ansible-community/ansible-build-data#258

Closed

gotmax23 force-pushed the announcement branch from 837316b to 4363f0d Compare February 5, 2024 16:01

felixfontein reviewed Feb 5, 2024

View reviewed changes

gotmax23 and others added 2 commits February 6, 2024 23:33

add announcements subcommand to gen release announcement text

f5b1c9b

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>

add basic functional tests for announcements command

83bd425

gotmax23 force-pushed the announcement branch from 9b36868 to 2f0287b Compare February 6, 2024 23:33

gotmax23 added 5 commits February 7, 2024 00:06

announcements: add --send to automatically send announcements

a14112f

This makes it easier to chain together the announcements and send-announcements command.

use typing_extensions.TypedDict to satisfy pydantic

01ff131

announcements: add docstrings

baad3e3

announcements: make pyperclip dependency optional

28fb8de

gotmax23 force-pushed the announcement branch from 2f0287b to 28fb8de Compare February 7, 2024 00:07

felixfontein reviewed Feb 7, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

Fix typo.

a5dfa08

felixfontein approved these changes Feb 7, 2024

View reviewed changes

felixfontein merged commit 146659e into ansible-community:main Feb 7, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add announcements subcommand to gen release announcement text #573

add announcements subcommand to gen release announcement text #573

gotmax23 commented Jan 25, 2024

webknjaz Jan 25, 2024

gotmax23 Jan 25, 2024 •

edited

webknjaz Jan 26, 2024

webknjaz Jan 25, 2024

gotmax23 Jan 25, 2024

webknjaz Jan 26, 2024

gotmax23 left a comment

gotmax23 Jan 25, 2024

gotmax23 Jan 25, 2024 •

edited

gotmax23 commented Jan 26, 2024

gotmax23 commented Jan 26, 2024

gotmax23 commented Jan 26, 2024

gotmax23 commented Jan 26, 2024 •

edited

gotmax23 commented Feb 1, 2024

webknjaz commented Feb 1, 2024

felixfontein Feb 5, 2024

gotmax23 Feb 6, 2024

gotmax23 Feb 6, 2024 •

edited

gotmax23 Feb 7, 2024

felixfontein Feb 7, 2024

felixfontein commented Feb 7, 2024

-        if not await asyncio.to_thread(file.is_file):
+        dist_file_exists = await asyncio.to_thread(file.is_file)
+        if not dist_file_exists:

add announcements subcommand to gen release announcement text #573

add announcements subcommand to gen release announcement text #573

Conversation

gotmax23 commented Jan 25, 2024

Choose a reason for hiding this comment

gotmax23 Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotmax23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotmax23 Jan 25, 2024 • edited

Choose a reason for hiding this comment

gotmax23 commented Jan 26, 2024

gotmax23 commented Jan 26, 2024

gotmax23 commented Jan 26, 2024

gotmax23 commented Jan 26, 2024 • edited

gotmax23 commented Feb 1, 2024

webknjaz commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotmax23 Feb 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Feb 7, 2024

gotmax23 Jan 25, 2024 •

edited

gotmax23 Jan 25, 2024 •

edited

gotmax23 commented Jan 26, 2024 •

edited

gotmax23 Feb 6, 2024 •

edited