{{p.is_open|yesno:"Active patches,Closed patches"}}
| {%if p.is_open%}Patch{%if sortkey == 0%} {%endif%}{%endif%} |
+ Patch{%if sortkey == 5%} {%endif%} |
+ ID{%if sortkey == 4%} {%endif%} |
Status | Ver | +CI status | Author | Reviewers | Committer | -{%if p.is_open%}Num cfs{%if sortkey == 3%} {%endif%}{%else%}Num cfs{%endif%} |
- {%if p.is_open%}Latest activity{%if sortkey == 1%} {%endif%}{%else%}Latest activity{%endif%} |
- {%if p.is_open%}Latest mail{%if sortkey == 2%} {%endif%}{%else%}Latest mail{%endif%} |
+ Num cfs{%if sortkey == 3%} {%endif%} |
+ Latest activity{%if sortkey == 1%} {%endif%} |
+ Latest mail{%if sortkey == 2%} {%endif%} |
{%if user.is_staff%}
Select | {%endif%} @@ -79,13 +81,38 @@
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| {{p.topic}} | |||||||||||||||
| {{p.topic}} | |||||||||||||||
| {{p.name}} | +{{p.name}} | +{{p.id}} | {{p.status|patchstatusstring}} | {%if p.targetversion%}{{p.targetversion}}{%endif%} | +
+ {%if not p.cfbot_results %}
+ Not processed
+ {%elif p.cfbot_results.needs_rebase %}
+
+ Needs rebase!
+
+ {%else%}
+ 
+
+ {%if p.cfbot_results.failed > 0 %}
+ 
+ {%elif p.cfbot_results.completed < p.cfbot_results.total %}
+ 
+ {%else%}
+ 
+ {%endif%}
+
+ {{p.cfbot_results.completed}}/{{p.cfbot_results.total}}
+
+
+ {%endif%}
+ |
{{p.author_names|default:''}} | {{p.reviewer_names|default:''}} | {{p.committer|default:''}} | diff --git a/pgcommitfest/commitfest/templates/mail/patch_notify.txt b/pgcommitfest/commitfest/templates/mail/patch_notify.txt index 1ab838c..5f3d744 100644 --- a/pgcommitfest/commitfest/templates/mail/patch_notify.txt +++ b/pgcommitfest/commitfest/templates/mail/patch_notify.txt @@ -5,7 +5,7 @@ have received updates in the PostgreSQL commitfest app: {{p.patch.name}} https://commitfest.postgresql.org/{{p.patch.patchoncommitfest_set.all.0.commitfest.id}}/{{p.patch.id}}/ {%for h in p.entries%} -* {{h.what}} ({{h.by}}){%endfor%} +* {{h.what}} by {{h.by_string()}}{%endfor%} {%endfor%} diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 33670cf..69d72bc 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -12,6 +12,43 @@Title | {{patch.name}} | |||||
| CI (CFBot) | +
+ {%if not cfbot_branch %}
+ Not processed
+ {%elif not cfbot_branch.commit_id %}
+
+ Needs rebase!
+ Additional links previous successfully applied patch (outdated):
+
+
+ Summary
+ {%else%}
+
+
+ Summary
+ {%for c in cfbot_tasks %}
+ {%if c.status == 'COMPLETED'%}
+
+ {%elif c.status == 'CREATED' %}
+ 
+ {%elif c.status == 'EXECUTING' %}
+
+ {%else %}
+
+ {%endif%}
+ {%endfor%}
+ {%endif%}
+ {%if cfbot_branch %}
+
+ {%endif%}
+
+ |
+ ||||||||||||||
| Topic | {{patch.topic}} | @@ -31,7 +68,7 @@||||||||||||||
| Status | {%for c in patch_commitfests %}
- {{c.commitfest}}: {{c.statusstring}}
+ {{c.commitfest}}: {{c.statusstring}}
{%endfor%}
|
||||||||||||||
| Links | -CFbot results (CirrusCI) - CFbot GitHub{%if patch.wikilink%} + {%if patch.wikilink%} Wiki{%endif%}{%if patch.gitlink%} Git {%endif%} | ||||||||||||||
| Checkout latest CFbot patchset | -
- Go to your local checkout of the PostgreSQL repository and run:
- |
- ||||||||||||||
| Emails |
@@ -138,7 +165,7 @@ Annotations | ||||||||||||||
| {{h.date}} | {{h.by_string}} | @@ -156,7 +183,9 @@
{%include "patch_commands.inc"%}
+
{%comment%}commit dialog{%endcomment%}
diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc
index 8f18f71..9b78a68 100644
--- a/pgcommitfest/commitfest/templates/patch_commands.inc
+++ b/pgcommitfest/commitfest/templates/patch_commands.inc
@@ -21,7 +21,7 @@
Rejected
Withdrawn
Returned with feedback
- Move to next CF
+ Move to next CF
Committed
@@ -37,4 +37,4 @@
{%endif%}
-
\ No newline at end of file
+
diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py
index 2c21cd3..4b52864 100644
--- a/pgcommitfest/commitfest/views.py
+++ b/pgcommitfest/commitfest/views.py
@@ -14,12 +14,13 @@
from email.mime.text import MIMEText
from email.utils import formatdate, make_msgid
import json
+import hmac
import urllib
from pgcommitfest.mailqueue.util import send_mail, send_simple_mail
from pgcommitfest.userprofile.util import UserWrapper
-from .models import CommitFest, Patch, PatchOnCommitFest, PatchHistory, Committer
+from .models import CommitFest, Patch, PatchOnCommitFest, PatchHistory, Committer, CfbotBranch, CfbotTask
from .models import MailThread
from .forms import PatchForm, NewPatchForm, CommentForm, CommitFestFilterForm
from .forms import BulkEmailForm
@@ -186,6 +187,10 @@ def commitfest(request, cfid):
orderby_str = 'lastmail, created'
elif sortkey == 3:
orderby_str = 'num_cfs DESC, modified, created'
+ elif sortkey == 4:
+ orderby_str = 'p.id'
+ elif sortkey == 5:
+ orderby_str = 'p.name, created'
else:
orderby_str = 'p.id'
sortkey = 0
@@ -213,7 +218,24 @@ def commitfest(request, cfid):
(poc.status=ANY(%(openstatuses)s)) AS is_open,
(SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_authors cpa ON cpa.user_id=auth_user.id WHERE cpa.patch_id=p.id) AS author_names,
(SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_reviewers cpr ON cpr.user_id=auth_user.id WHERE cpr.patch_id=p.id) AS reviewer_names,
-(SELECT count(1) FROM commitfest_patchoncommitfest pcf WHERE pcf.patch_id=p.id) AS num_cfs
+(SELECT count(1) FROM commitfest_patchoncommitfest pcf WHERE pcf.patch_id=p.id) AS num_cfs,
+(
+ SELECT row_to_json(t) as cfbot_results
+ from (
+ SELECT
+ count(*) FILTER (WHERE task.status = 'COMPLETED') as completed,
+ count(*) FILTER (WHERE task.status in ('CREATED', 'SCHEDULED', 'EXECUTING')) running,
+ count(*) FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) failed,
+ count(*) total,
+ string_agg(task.task_name, ', ') FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) as failed_task_names,
+ branch.commit_id IS NULL as needs_rebase,
+ branch.apply_url
+ FROM commitfest_cfbotbranch branch
+ LEFT JOIN commitfest_cfbottask task ON task.branch_id = branch.branch_id
+ WHERE branch.patch_id=p.id
+ GROUP BY branch.patch_id
+ ) t
+)
FROM commitfest_patch p
INNER JOIN commitfest_patchoncommitfest poc ON poc.patch_id=p.id
INNER JOIN commitfest_topic t ON t.id=p.topic_id
@@ -298,19 +320,23 @@ def global_search(request):
})
-def patch_redirect(request, patchid):
- last_commitfest = PatchOnCommitFest.objects.select_related('commitfest').filter(patch_id=patchid).order_by('-commitfest__startdate').first()
- if not last_commitfest:
- raise Http404("Patch not found")
- return HttpResponseRedirect(f'/{last_commitfest.commitfest_id}/{patchid}/')
+def patch_legacy_redirect(request, cfid, patchid):
+ # Previously we would include the commitfest id in the URL. This is no
+ # longer the case.
+ return HttpResponseRedirect(f'/patch/{patchid}/')
-def patch(request, cfid, patchid):
- cf = get_object_or_404(CommitFest, pk=cfid)
- patch = get_object_or_404(Patch.objects.select_related(), pk=patchid, commitfests=cf)
- patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate')
+def patch(request, patchid):
+ patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+
+ patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate').all()
+ cf = patch_commitfests[0].commitfest
+
committers = Committer.objects.filter(active=True).order_by('user__last_name', 'user__first_name')
+ cfbot_branch = getattr(patch, 'cfbot_branch', None)
+ cfbot_tasks = patch.cfbot_tasks.order_by('position') if cfbot_branch else []
+
# XXX: this creates a session, so find a smarter way. Probably handle
# it in the callback and just ask the user then?
if request.user.is_authenticated:
@@ -333,6 +359,8 @@ def patch(request, cfid, patchid):
'cf': cf,
'patch': patch,
'patch_commitfests': patch_commitfests,
+ 'cfbot_branch': cfbot_branch,
+ 'cfbot_tasks': cfbot_tasks,
'is_committer': is_committer,
'is_this_committer': is_this_committer,
'is_reviewer': is_reviewer,
@@ -346,9 +374,9 @@ def patch(request, cfid, patchid):
@login_required
@transaction.atomic
-def patchform(request, cfid, patchid):
- cf = get_object_or_404(CommitFest, pk=cfid)
- patch = get_object_or_404(Patch, pk=patchid, commitfests=cf)
+def patchform(request, patchid):
+ patch = get_object_or_404(Patch, pk=patchid)
+ cf = patch.current_commitfest()
prevreviewers = list(patch.reviewers.all())
prevauthors = list(patch.authors.all())
@@ -404,7 +432,7 @@ def newpatch(request, cfid):
# Now add the thread
try:
doAttachThread(cf, patch, form.cleaned_data['threadmsgid'], request.user)
- return HttpResponseRedirect("/%s/%s/edit/" % (cf.id, patch.id))
+ return HttpResponseRedirect("/patch/%s/edit/" % (patch.id,))
except Http404:
# Thread not found!
# This is a horrible breakage of API layers
@@ -438,21 +466,12 @@ def _review_status_string(reviewstatus):
@login_required
@transaction.atomic
-def comment(request, cfid, patchid, what):
- cf = get_object_or_404(CommitFest, pk=cfid)
+def comment(request, patchid, what):
patch = get_object_or_404(Patch, pk=patchid)
+ cf = patch.current_commitfest()
poc = get_object_or_404(PatchOnCommitFest, patch=patch, commitfest=cf)
is_review = (what == 'review')
- if poc.is_closed:
- # We allow modification of patches in closed CFs *only* if it's the
- # last CF that the patch is part of. If it's part of another CF, that
- # is later than this one, tell the user to go there instead.
- lastcf = PatchOnCommitFest.objects.filter(patch=patch).order_by('-commitfest__startdate')[0]
- if poc != lastcf:
- messages.add_message(request, messages.INFO, "The status of this patch cannot be changed in this commitfest. You must modify it in the one where it's open!")
- return HttpResponseRedirect('..')
-
if request.method == 'POST':
try:
form = CommentForm(patch, poc, is_review, data=request.POST)
@@ -535,17 +554,10 @@ def comment(request, cfid, patchid, what):
@login_required
@transaction.atomic
-def status(request, cfid, patchid, status):
- poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cfid, patch__id=patchid)
-
- if poc.is_closed:
- # We allow modification of patches in closed CFs *only* if it's the
- # last CF that the patch is part of. If it's part of another CF, that
- # is later than this one, tell the user to go there instead.
- lastcf = PatchOnCommitFest.objects.filter(patch__id=patchid).order_by('-commitfest__startdate')[0]
- if poc != lastcf:
- messages.add_message(request, messages.INFO, "The status of this patch cannot be changed in this commitfest. You must modify it in the one where it's open!")
- return HttpResponseRedirect('/%s/%s/' % (poc.commitfest.id, poc.patch.id))
+def status(request, patchid, status):
+ patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+ cf = patch.current_commitfest()
+ poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cf.id, patch__id=patchid)
if status == 'review':
newstatus = PatchOnCommitFest.STATUS_REVIEW
@@ -565,22 +577,29 @@ def status(request, cfid, patchid, status):
PatchHistory(patch=poc.patch, by=request.user, what='New status: %s' % poc.statusstring).save_and_notify()
- return HttpResponseRedirect('/%s/%s/' % (poc.commitfest.id, poc.patch.id))
+ return HttpResponseRedirect('/patch/%s/' % (poc.patch.id))
@login_required
@transaction.atomic
-def close(request, cfid, patchid, status):
- poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cfid, patch__id=patchid)
-
- if poc.is_closed:
- # We allow modification of patches in closed CFs *only* if it's the
- # last CF that the patch is part of. If it's part of another CF, that
- # is later than this one, tell the user to go there instead.
- lastcf = PatchOnCommitFest.objects.filter(patch__id=patchid).order_by('-commitfest__startdate')[0]
- if poc != lastcf:
- messages.add_message(request, messages.INFO, "The status of this patch cannot be changed in this commitfest. You must modify it in the one where it's open!")
- return HttpResponseRedirect('/%s/%s/' % (poc.commitfest.id, poc.patch.id))
+def close(request, patchid, status):
+ patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+ cf = patch.current_commitfest()
+
+ try:
+ request_cfid = int(request.GET.get('cfid', ''))
+ except ValueError:
+ # int() failed, ignore
+ request_cfid = None
+
+ if request_cfid is not None and request_cfid != cf.id:
+ # The cfid parameter is only added to the /next/ link. That's the only
+ # close operation where two people pressing the button at the same time
+ # can have unintended effects.
+ messages.error(request, "The patch was moved to a new commitfest by someone else. Please double check if you still want to retry this operation.")
+ return HttpResponseRedirect('/%s/%s/' % (cf.id, patch.id))
+
+ poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cf.id, patch__id=patchid)
poc.leavedate = datetime.now()
@@ -668,8 +687,7 @@ def close(request, cfid, patchid, status):
@login_required
@transaction.atomic
-def reviewer(request, cfid, patchid, status):
- get_object_or_404(CommitFest, pk=cfid)
+def reviewer(request, patchid, status):
patch = get_object_or_404(Patch, pk=patchid)
is_reviewer = request.user in patch.reviewers.all()
@@ -688,7 +706,6 @@ def reviewer(request, cfid, patchid, status):
@login_required
@transaction.atomic
def committer(request, cfid, patchid, status):
- get_object_or_404(CommitFest, pk=cfid)
patch = get_object_or_404(Patch, pk=patchid)
committer = list(Committer.objects.filter(user=request.user, active=True))
@@ -713,8 +730,7 @@ def committer(request, cfid, patchid, status):
@login_required
@transaction.atomic
-def subscribe(request, cfid, patchid, sub):
- get_object_or_404(CommitFest, pk=cfid)
+def subscribe(request, patchid, sub):
patch = get_object_or_404(Patch, pk=patchid)
if sub == 'un':
@@ -727,6 +743,12 @@ def subscribe(request, cfid, patchid, sub):
return HttpResponseRedirect("../")
+def send_patch_email(request, patchid):
+ patch = get_object_or_404(Patch, pk=patchid)
+ cf = patch.current_commitfest()
+ return send_email(request, cf.id)
+
+
@login_required
@transaction.atomic
def send_email(request, cfid):
@@ -788,6 +810,137 @@ def _user_and_mail(u):
})
+@transaction.atomic
+def cfbot_ingest(message):
+ """Ingest a single message status update message receive from cfbot. It
+ should be a Python dictionary, decoded from JSON already."""
+
+ cursor = connection.cursor()
+
+ branch_status = message["branch_status"]
+ patch_id = branch_status["submission_id"]
+ branch_id = branch_status["branch_id"]
+
+ try:
+ patch = Patch.objects.get(pk=patch_id)
+ except Patch.DoesNotExist:
+ # If the patch doesn't exist, there's nothing to do. This should never
+ # happen in production, but on the test system it's possible because
+ # not it doesn't contain the newest patches that the CFBot knows about.
+ return
+
+ # Every message should have a branch_status, which we will INSERT
+ # or UPDATE. We do this first, because cfbot_task refers to it.
+ # Due to the way messages are sent/queued by cfbot it's possible that it
+ # sends the messages out-of-order. To handle this we we only update in two
+ # cases:
+ # 1. The created time of the branch is newer than the one in our database:
+ # This is a newer branch
+ # 2. If it's the same branch that we already have, but the modified time is
+ # newer: This is a status update for the current branch that we received
+ # in-order.
+ cursor.execute("""INSERT INTO commitfest_cfbotbranch (patch_id, branch_id,
+ branch_name, commit_id,
+ apply_url, status,
+ created, modified)
+ VALUES (%s, %s, %s, %s, %s, %s, %s, %s)
+ ON CONFLICT (patch_id) DO UPDATE
+ SET status = EXCLUDED.status,
+ modified = EXCLUDED.modified,
+ branch_id = EXCLUDED.branch_id,
+ branch_name = EXCLUDED.branch_name,
+ commit_id = EXCLUDED.commit_id,
+ apply_url = EXCLUDED.apply_url,
+ created = EXCLUDED.created
+ WHERE commitfest_cfbotbranch.created < EXCLUDED.created
+ OR (commitfest_cfbotbranch.branch_id = EXCLUDED.branch_id
+ AND commitfest_cfbotbranch.modified < EXCLUDED.modified)
+ """,
+ (
+ patch_id,
+ branch_id,
+ branch_status["branch_name"],
+ branch_status["commit_id"],
+ branch_status["apply_url"],
+ branch_status["status"],
+ branch_status["created"],
+ branch_status["modified"])
+ )
+
+ # Now we check what we have in our database. If that contains a different
+ # branch_id than what we just tried to insert, then apparently this is a
+ # status update for an old branch and we don't care about any of the
+ # contents of this message.
+ branch_in_db = CfbotBranch.objects.get(pk=patch_id)
+ if branch_in_db.branch_id != branch_id:
+ return
+
+ # Most messages have a task_status. It might be missing in rare cases, like
+ # when cfbot decides that a whole branch has timed out. We INSERT or
+ # UPDATE.
+ if "task_status" in message:
+ task_status = message["task_status"]
+ cursor.execute("""INSERT INTO commitfest_cfbottask (task_id, task_name, patch_id, branch_id,
+ position, status,
+ created, modified)
+ VALUES (%s, %s, %s, %s, %s, %s, %s, %s)
+ ON CONFLICT (task_id) DO UPDATE
+ SET status = EXCLUDED.status,
+ modified = EXCLUDED.modified
+ WHERE commitfest_cfbottask.modified < EXCLUDED.modified""",
+ (
+ task_status["task_id"],
+ task_status["task_name"],
+ patch_id,
+ branch_id,
+ task_status["position"],
+ task_status["status"],
+ task_status["created"],
+ task_status["modified"])
+ )
+
+ # Remove any old tasks that are not related to this branch. These should
+ # only be left over when we just updated the branch_id. Knowing if we just
+ # updated the branch_id was is not trivial though, because INSERT ON
+ # CONFLICT does not allow us to easily return the old value of the row.
+ # So instead we always delete all tasks that are not related to this
+ # branch. This is fine, because doing so is very cheap in the no-op case
+ # because we have an index on patch_id and there's only a handful of tasks
+ # per patch.
+ cursor.execute("DELETE FROM commitfest_cfbottask WHERE patch_id=%s AND branch_id != %s", (patch_id, branch_id))
+
+ # We change the needs_rebase field using a separate UPDATE because this way
+ # we can find out what the previous state of the field was (sadly INSERT ON
+ # CONFLICT does not allow us to return that). We need to know the previous
+ # state so we can skip sending notifications if the needs_rebase status did
+ # not change.
+ needs_rebase = branch_status['commit_id'] is None
+ if bool(branch_in_db.needs_rebase_since) is not needs_rebase:
+ if needs_rebase:
+ branch_in_db.needs_rebase_since = datetime.now()
+ else:
+ branch_in_db.needs_rebase_since = None
+ branch_in_db.save()
+
+ if needs_rebase:
+ PatchHistory(patch=patch, by=None, by_cfbot=True, what='Patch needs rebase').save_and_notify(authors_only=True)
+ else:
+ PatchHistory(patch=patch, by=None, by_cfbot=True, what='Patch does not need rebase anymore').save_and_notify(authors_only=True)
+
+
+@csrf_exempt
+def cfbot_notify(request):
+ if request.method != 'POST':
+ return HttpResponseForbidden("Invalid method")
+
+ j = json.loads(request.body)
+ if not hmac.compare_digest(j['shared_secret'], settings.CFBOT_SECRET):
+ return HttpResponseForbidden("Invalid API key")
+
+ cfbot_ingest(j)
+ return HttpResponse(status=200)
+
+
@csrf_exempt
def thread_notify(request):
if request.method != 'POST':
diff --git a/pgcommitfest/local_settings_example.py b/pgcommitfest/local_settings_example.py
index d3648cc..31da1f0 100644
--- a/pgcommitfest/local_settings_example.py
+++ b/pgcommitfest/local_settings_example.py
@@ -1,3 +1,5 @@
+import os
+
# Enable more debugging information
DEBUG = True
# Prevent logging to try to send emails to postgresql.org admins.
@@ -22,3 +24,7 @@
# It's not great, because it won't redirect to the page you were trying to
# access, but it's better than a HTTP 500 error.
PGAUTH_REDIRECT = '/admin/login/'
+
+MOCK_ARCHIVES = True
+BASE_DIR = os.path.dirname(os.path.abspath(__file__))
+MOCK_ARCHIVE_DATA = os.path.join(BASE_DIR, 'commitfest', 'fixtures', 'archive_data.json')
diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py
index 53fce1c..733fcc5 100644
--- a/pgcommitfest/urls.py
+++ b/pgcommitfest/urls.py
@@ -19,23 +19,31 @@
re_path(r'^(\d+)/$', views.commitfest),
re_path(r'^(open|inprogress|current)/(.*)$', views.redir),
re_path(r'^(?P