Is the API for flagging a pull request now deprecated in favor of flagging a commit.

Or put another way, do the two operate on the same foundation? Is flagging a pull request really implemented as flagging the HEAD of pull request branch?

s/pull request page/pages where this flag is displayed/

s/pull-request page/pages where this flag is displayed/

Both APIs are still present and supported, one shows up on the commit page the other only on the PR page.

It's an idea indeed to also show commits flags on PR pages, but will have to see how to present the info (that's the hardest part)

Could we document "special" percentages? Does 0% mean "pending" and 100% mean done?

How is failure vs success vs error represented?

Commit page indeed

100% is passed, 0% is failed, there is no distinction of failure vs error

I don't see changes to pull request code. Does that mean that pull request flags are tied to the pull request itself, and not to the current HEAD commit of the pull request branch?

That is to say there are two ways of flagging a pull request:

1. Flag the pull request itself, these flags persist across commit changes on the pull request.
2. Flag the HEAD commit of the pull request. These flags apply to the pull request only as long as that commit is at the HEAD of the pull request branch.

Obviosuly the second one applies to non-pull requests too.

(Reply button is gone) ... I'm pretty sure you'll want to display commit flags for a PR's HEAD commit on the PR page, much like GitHub and GitLab do. But it makes sense to do that work as a follow up.

Flag the pull request itself, these flags persist across commit changes on the pull request.

This is the current behavior :)

Commit flags are linked to a commit regardless of them being included in a PR or not

I think we're missing a state/status field ... and the percentage stuff doesn't go far enough, and it's going to be a pain to use ... something we work around over and over and over again.

See GitLab: https://docs.gitlab.com/ce/api/commits.html#post-the-build-status-to-a-commit

state: The state of the status. Can be one of the following: pending, running, success, failed, canceled

And GitHub: https://developer.github.com/v3/repos/statuses/#create-a-status

state: Required. The state of the status. Can be one of error, failure, pending, or success

I'm pretty sure you'll want to display commit flags for a PR's HEAD commit on the PR page, much like GitHub and GitLab do.

Sounds like a good idea :)

But it makes sense to do that work as a follow up.

+1 thanks

Are UIDs unique per flag context (ie: commit or PR)? Or globally unique. May be worth clarifying that in the documentation.

Should be ```WHERE percent not in (100, \'000\')')

oups, indeed :)

Are you sure this is necessary?

It is workable, but IMO a better solution would be to scope this UID to the commit/PR ... instead of globally.

And yes, this is what the "competition" does. Again, not the end of the world, but this does make callers more complex than they need to be.

How does access control work here? What if I specify the uid for a commit that my user didn't post?

How does access control work here? What if I specify the uid for a commit that my user didn't post?

Well the API token to flag commit/PR is per repo and you can only get one if you have access to the project.

There are cross-project API tokens but these can be given out to people only by instance wide pagure admin.

It is workable, but IMO a better solution would be to scope this UID to the commit/PR ... instead of globally

Changing this is likely doable but means more changes at the DB level, let me see if I can come up with something.

I wonder if calling this a flag is the best choice. To me, "flag" feels very binary, whereas we have multiple free form fields here as well as a url to link further content.

Should there be a check here to see if the status is one of the valid values? While not checking might be better for future changes, catching errors like that might actually help debug if someone accidentally tries to push the wrong status via a typo.

since all other variables here are named, why not name session also?
session=SESSION

Is there a reason here that there are inconsistencies between commitid and commit_hash? It's a nitpick, but maybe there's more to it than I'm seeing.

     Should there be a check here to see if the status is one of the valid values? While not checking might be better for future changes, catching errors like that might actually help debug if someone accidentally tries to push the wrong status via a typo.

Do the form validators cover this? https://pagure.io/pagure/pull-request/2772#_12,22
Or do those not cover calling the API as a REST API?

Overall this looks good, and I'm very much looking forward to adding status info to commits.

That's taken care of by the form :)

Sure can do :)

I can change commit_hash to commitid, but I thought commit_hash was more explicit for the DB field.

I don't think it's critical, it just surprised me. Keeping commit_hash in the database is better I believe, just like you said.

Since this is all new code I've changed commitid to commit_hash :)

While I understand this, this is the terminology we're already using for PRs so I think it's better to remain consistent.

dictionary

What is this template used for exactly? The percentages are mapped differently to failure/success above, I think (0 is failure, >0 is success)

Why not also call this commit_hash? That saves the extra explanation.

Minor nitpicks, looks good otherwise. +1 once those are addressed

Indeed, good catch

I'll put that in its own commit since the typo is in several places

Oh, shoot, forgot to update this part, as well in the PR template.

Mind if I do this in another PR so the diff is lower?

I addressed all your points except for showing the flag in the template using status instead of the percentage.

Since this PR is getting long, I'm going to merge it as is and address that point in a new PR, should be easier to review :)

Thanks for your reviews folks!

While working on the changes to the templates, I found this bug:

diff --git a/alembic/versions/2b626a16542e_commit_flag.py b/alembic/versions/2b626a16542e_commit_flag.py
index 872bc49..58c5e47 100644
--- a/alembic/versions/2b626a16542e_commit_flag.py
+++ b/alembic/versions/2b626a16542e_commit_flag.py
@@ -40,7 +40,7 @@ def upgrade():
             nullable=False, index=True),
         sa.Column('status', sa.String(32), nullable=False),
         sa.Column('username', sa.Text(), nullable=True),
-        sa.Column('percent', sa.Integer(), nullable=False),
+        sa.Column('percent', sa.Integer(), nullable=True),
         sa.Column('comment', sa.Text(), nullable=False),
         sa.Column('url', sa.Text(), nullable=False),
         sa.Column(

So since this PR isn't merged yet, I'm pushing the change here and I'll merge.

Thanks!