Introduce Drafts page and automatic CF creation #70

JelteF · 2025-06-09T10:21:57Z

This introduce a new type of CommitFest a "Draft" CommitFest. This
CommitFest is never "In Progress", but it can be open. It exists for a
year. It opens when the final regular CommitFest of the year becomes "In
Progress" and stays open for exactly a year. It never becomes "In
Progress" itself.

Adding a second type of CommitFest also needed a redesign of quite a few
things, like the homepage. Also management of the CommitFests needed to
be made a bit easier, so admins don't forget to close/create Draft
CommitFests. So now, closing/opening/creating CommitFests is done
automatically when the time is right for that. A help page is also
introduced to explain the CommitFest app.

The naming of CommitFests has been changed too. Since we now have a
Draft CF every year that needs a name, it seemed reasonable to align the
names of the other CFs with that too. So each PG release cycle now has 5
regular commitfests that are called:

PG18-1
PG18-2
PG18-3
PG18-4
PG18-Final

And a single Draft CommitFest, called:

PG18-Draft

Finally, it also adds a small initial API endpoint for the CFBot, to
request the commitfests that need CI runs. Future PRs will extend this
API surface to also include/allow requesting the actual patches that CI
should run on.

Fixes #25
Fixes #65

JelteF · 2025-06-09T10:22:41Z

pgcommitfest/commitfest/templates/help.html

+  A "patch" is a bit of an overloaded term in the PostgreSQL community. Email threads on the mailing list often contain "patch files" as attachments, such a file is often referred to as a "patch". A single email can even contain multiple related "patch files", which are called a "patchset". However, in the context of a CommitFest app a "patch" usually means a "patch entry" in the CommitFest app. Such a "patch entry" is a reference to a mailinglist thread on which change to PostgreSQL has been proposed, by someone sending an email that contain one or more "patch files". The CommitFest app will automatically detect new versions of the patch files and update the "patch entry" accordingly.
+ </p>
+ <p>
+  There are three active categories of patch status:


TODO: Somewhere should be "Returned with Feedback"

In my mind it should either be rejected (or, ideally, withdrawn) or moved back into drafts. Trying to reduce the amount of choice presented to the user by omitting RfW - which isn’t really a descriptive “end state”. If no one accepts that logic it can be easily added.

I think the states that we have need an overhaul at some point in the not too far future. But I think that should wait until people have gotten used to tags a bit more. So for now I've simply documented RfW in the way that it is currently being used.

This introduce a new type of CommitFest a "Draft" CommitFest. This CommitFest is never "In Progress", but it can be open. It exists for a year. It opens when the final regular CommitFest of the year becomes "In Progress" and stays open for exactly a year. It never becomes "In Progress" itself. Adding a second type of CommitFest also needed a redesign of quite a few things, like the homepage. Also management of the CommitFests needed to be made a bit easier, so admins don't forget to close/create Draft CommitFests. So now, closing/opening/creating CommitFests is done automatically when the time is right for that. A help page is also introduced to explain the CommitFest app. The naming of CommitFests has been changed too. Since we now have a Draft CF every year that needs a name, it seemed reasonable to align the names of the other CFs with that too. So each PG release cycle now has 5 regular commitfests that are called: - PG18-1 - PG18-2 - PG18-3 - PG18-4 - PG18-Final And a single Draft CommitFest, called: - PG18-Draft Finally, it also adds a small initial API endpoint for the CFBot, to request the commitfests that need CI runs. Future PRs will extend this API surface to also include/allow requesting the actual patches that CI should run on. Co-Authored-By: David G. Johnston <[email protected]>

polobo · 2025-06-09T13:09:23Z

Did you have specific rationale for choosing to add a draft Boolean instead of using a status?

polobo · 2025-06-09T13:24:24Z

pgcommitfest/commitfest/templates/patch_commands.inc

We push patches written during an in progress cf all the time. Those should be able to be added to the in progress cf by a committer so their cf of record is correct. I.e., move to should show in progress conditionally.

Interesting... Currently that's not done, but indeed that seems reasonable. I think the easiest way would be to automatically move a patch if it's "Committed" in an "Open" commitfest, to the "In Progress" commitfest if that exists.

I think the easiest way would be to automatically move a patch if it's "Committed" in an "Open" commitfest, to the "In Progress" commitfest if that exists.

I implemented this now.

polobo · 2025-06-09T13:38:02Z

Not a fan of the fundamental relevant cf getter method having non-cache related side effects that only ever trigger every other month or so. It isn’t even a pure date-related action; our final cf in particular has an end date that is changed during the months it happens in. We should,have a form to make the action easy to perform, but not remove the human component altogether.

JelteF · 2025-06-09T18:14:07Z

Did you have specific rationale for choosing to add a draft Boolean instead of using a status?

Primarily because being a draft commitfest is a property of the commitfest, not a state it can be in. Once a draft commitfest is closed, it's still a draft commit fest. If we make it a state the only way of knowing that is based on the name. By using a boolean we can also have the full list of commitfests page differentiate between draft commitfests and regular commitfests some way.

Not a fan of the fundamental relevant cf getter method having non-cache related side effects that only ever trigger every other month or so.

I agree it's a bit uncommon to do like this, but I don't see the benefit of requiring a person to press a button every 2 months.

It isn’t even a pure date-related action; our final cf in particular has an end date that is changed during the months it happens in.

The feature freeze date is decided by the RMT well before the end of March. It seems fine to have them change the enddate on the commitfest app when they email the feature freeze date.

polobo · 2025-06-09T18:19:54Z

Yeah, I’m ok with draft as a Boolean.

Can we just setup a cron job to run daily to do the switchover then? I’d probably still keep the button to press for testing or a fall-back.

JelteF · 2025-06-09T18:23:10Z

Can we just setup a cron job to run daily to do the switchover then? I’d probably still keep the button to press for testing or a fall-back.

That's pretty much what the CFBot does now, except it's an "every minute" cron job. I guess I could make the refreshing only occur for the API, but I don't really understand what the actual problem is you have with this approach.

polobo · 2025-06-09T18:30:34Z

Mainly the fundamental POLA violation of having a read-only query modifying state. And why spend overhead on every request checking whether said update is needed or not when 99+% of the time it will not be. Also, it seems problematic to have it be the only way this process gets executed; the fact you have to add a disable flag is just one such consequence. The design is just really an unappealing and unclean combining of unrelated features.

…atuses

polobo · 2025-06-09T18:42:36Z

pgcommitfest/commitfest/templates/help.html

  </ul>
-  And there are three preferred inactive categories of patch status for when
-  a patch has been resolved, either by being committed or not:
+  And there four "Closed" categories of patch status for when a patch has been


And there [are] four “Closed”…

…ird way

JelteF · 2025-06-09T18:52:08Z

Mainly the fundamental POLA violation of having a read-only query modifying state.

So I kind of agree with that. And if it'd have been easy for me to create a cron job on the machine, I'd probably have done that. Sadly that's not the situation we're in. Getting anything to happen on the infrastructure side is a huge hassle. So I try to avoid that as much as possible.

I added code comment with that reasoning. And I'll leave it like this.

And why spend overhead on every request checking whether said update is needed or not when 99+% of the time it will not be.

The overhead of the check is extremely minimal. So I'm not worried about that at all. It's only 5 comparisons.

polobo · 2025-06-09T18:57:22Z

I added code comment with that reasoning. And I'll leave it like this.

I'll pay the tech debt down if I get annoyed enough. I do appreciate the decision in the face of our organizational dynamics.

…ommitted

polobo · 2025-06-09T19:21:37Z

pgcommitfest/commitfest/views.py

@@ -1053,6 +1053,17 @@ def close(request, patchid, status):
                by=request.user,
                what="Changed committer to %s" % committer,
            ).save_and_notify(prevcommitter=prevcommitter)
+
+        if poc.is_open:


I wanted to make it a policy that committed patches never appear in Drafts - really just open and withdrawn should (though maybe rejected/RfW too - but these likely won't happen in practice anyway). I was considering it acceptable for a non-draft open CF to have commits. Thus a committed patch not within the in progress cf should be moved there if it exists. Otherwise a draft patch should be moved to the current non-draft open cf and then committed there. (Since we report end-of-CF metrics moving stuff into a closed CF should be avoided.)

That's not what you said before

We push patches written during an in progress cf all the time. Those should be able to be added to the in progress cf by a committer so their cf of record is correct. I.e., move to should show in progress conditionally.

But regarding this:

Since we report end-of-CF metrics moving stuff into a closed CF should be avoided.

At the last developer meeting many committers were even in favor of allowing people to add patches to an in-progress commitfest. I think moving committed patches there is pretty reasonable. It seems silly and confusing to say that it was committed in the later commitfest, even if it actually managed to get in during the previous one. It seems especially confusing for the final commitfest of the year, i.e. some patches would show up as being committed as part of PG19-1, but are actually part of PG18.

I'll call his out though in the announcment email on the mailinglist. If enough people think this is wrong I'll revert.

"moving stuff into a closed CF" literally means the CF entry has a status of "Closed" - not "In Progress". We can move stuff into an "In Progress" status CF just fine - if one exists, which it doesn't half of the time... - in which case Draft patches being committed must first be moved to the guaranteed "Open" status CF (they likely are there already if being committed - we could instead prohibit "Commit" action while a patch is in a Draft CF.

I'm OK with moving stuff into a "Closed" CF actually; figured others wouldn't be though.

Ah alright, I misunderstood/misread. Let me restate what you're saying so I'm sure I understand correctly now: The current logic is good, but it should be amended that if something is committed in a Draft it should be moved to the "In Progress" commitfest (which already happens now), and if that doesn't exist it should be moved to the regular "Open" commitfest.

Put differently - should we have, right now, a "PG18-Beta" CF that is "In Progress" that all these new commits being made today get recorded within? Or do they get committed to PG18-Final (Closed) or PG19-1 (Open)?

if something is committed in a Draft it should be moved to the "In Progress" commitfest (which already happens now), and if that doesn't exist it should be moved to the regular "Open" commitfest.

Yes.

Yes.

I implemented that now.

should we have, right now, a "PG18-Beta" CF that is "In Progress" that all these new commits being made today get recorded within? or PG19-1 (Open)?

I think that's an interesting question. I think immediately creating an "In Progress", "PG18-Beta" CF makes sense once the "PG18-Final" CF is closed.

Or do they get committed to PG18-Final (Closed)

I think committing into an actually closed CF seems unexpected/strange (like you also said before).

or PG19-1 (Open)?

I think it makes sense to stick with this for now though, since it's the status quo. And I wouldn't want to introduce a Beta CF now, maybe for PG19 though.

Agreed with this being a year out; was using PG18 as an easy example.

polobo · 2025-06-10T00:24:04Z

pgcommitfest/commitfest/templates/patch_commands.inc

+       <a
+        href="move/?from_cf_id={{cf.id}}&to_cf_id={{cfs.draft.id}}">
+        {%if cf.draft %}
+         Next Drafts:


"Next" drafts doesn't make sense. There is never a next one that exists, only the current one. I suppose "Next CF" does still make sense though I really figured the name would be adequate for identification. If the patch is in the in progress cf you see both but know one is draft and one is non-draft readily from the names. If you aren't in the in progress cf you only see one of them anyway; and its never the "current" one. IOW, printing the name alone seems sufficient.

polobo · 2025-06-10T00:24:14Z

It never becomes "In Progress" itself.

For this, a check constraint should be enough to enforce it at the database layer.

For "a patch in a draft cf may not take on the Committed status" I believe we would need a trigger not unlike the "future prohibits patches" one that was part of the initial PR.

JelteF commented Jun 9, 2025

View reviewed changes

JelteF force-pushed the introduce-drafts-jelte branch from b9dddce to f05f885 Compare June 9, 2025 10:28

JelteF force-pushed the introduce-drafts-jelte branch from f05f885 to 90792d1 Compare June 9, 2025 10:33

This was referenced Jun 9, 2025

Introduce Workflow #65

Closed

Add a Parking Lot (Drafts) #53

Closed

Don't automatically create commitfests in dev environments

570b787

polobo reviewed Jun 9, 2025

View reviewed changes

Add "Returned with Feedback" and expand explanation of other patch st…

975e365

…atuses

polobo reviewed Jun 9, 2025

View reviewed changes

Add commet about reasoning why we update commitfests in this kinda we…

4fd357f

…ird way

Missing word in help page

a29aa00

Automatically move patches in open CF to in progress CF if they get c…

4c4a2c6

…ommitted

polobo reviewed Jun 9, 2025

View reviewed changes

JelteF added 2 commits June 9, 2025 21:28

Fix bug setting committer

f6c61e7

Automatically move committed draft patches to the open commitfest

96b0469

polobo reviewed Jun 10, 2025

View reviewed changes

Introduce Drafts page and automatic CF creation #70

Are you sure you want to change the base?

Introduce Drafts page and automatic CF creation #70

Uh oh!

Conversation

JelteF commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polobo commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polobo commented Jun 9, 2025

Uh oh!

JelteF commented Jun 9, 2025

Uh oh!

polobo commented Jun 9, 2025

Uh oh!

JelteF commented Jun 9, 2025

Uh oh!

polobo commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelteF commented Jun 9, 2025

Uh oh!

polobo commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polobo Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polobo commented Jun 10, 2025

Uh oh!

Uh oh!

polobo commented Jun 9, 2025 •

edited

Loading

polobo Jun 9, 2025 •

edited

Loading