Messaging: Set the agent_name, usernames and groups properties #3099

abompard · 2024-01-11T14:03:49Z

This sets a few properties that are standard in Fedora Messaging schemas and will allow notifications to be emitted by FMN.

messaging/copr_messaging/private/hierarchy.py

messaging/copr_messaging/private/schema_old.py

messaging/copr_messaging/private/schema_stomp_old.py

messaging/copr_messaging/schema.py

praiskup · 2024-01-12T08:33:59Z

messaging/copr_messaging/schema.py

@@ -58,6 +65,10 @@
            self.chroot,
        )

+    @property
+    def agent_name(self):


The linter probably assigned this warning to a wrong position in code (more methods with the same name).

messaging/copr_messaging/private/hierarchy.py

praiskup · 2024-01-12T08:33:04Z

messaging/copr_messaging/schema.py

+        """The username who caused the action that generated this message."""
+        if self.status == "canceled":
+            return self.submitter
+        # The other statuses are not caused by the user who started the build


Strictly speaking, anyone who has build or admin access to the project should be able to cancel the build. I don't think we have the needed info here.

Indeed, would it be possible to add that info to the message body?

I think so, but this seems like a separate issue (the asynchronous cancelation on the remote machine is relatively complicated logic).

So, does it make sense to mix up the submitter with the person who canceled the build? If yes, I think this PR is good to go. Otherwise, we should do something about it first, ideas?

Fixed! Thanks.

To be safe I didn't assume the submitter was the one who canceled the build. This may create notifications for people who didn't want to get notified of their own actions, but that's a lesser evil I think.

praiskup · 2024-01-12T08:35:18Z

Thank you for working on this, @abompard!

I'm not sure how precise we have to be here. E.g. what's the semantics of the username or usernames, who is going to react on this? The field will often be None because the submitter is unknown (because the build is triggered by webhook).

abompard · 2024-01-12T08:45:25Z

Basically, in FMN there's a rule called "My Events", that will emit a notification when the user name in the message's usernames property. So the rule of thumb is that if you, as an app developer, think your users should be notified of the action that emitted the message, you should put their name in the usernames property.

This sets a couple properties that are standard in Fedora Messaging schemas and will allow notifications to be emitted by FMN. Signed-off-by: Aurélien Bompard <[email protected]>

praiskup

Thank you.

knurd · 2024-01-16T09:22:28Z

you should put their name in the usernames property.

FWIW a quick comment, as I foresee a problem here (but maybe my foresight is wrong, so take this with a gain of salt and simply ignore me if I'm talking rubbish!):

In the old notifications systems the messages I got from gitlab webhook triggered builds were apparently referencing "gitlab.com:knurd42" as username (that's my gitlab account). Will there be a translation for this (doesn't look like it from the patch, but I might have easily missed something) or can I subscribe to that username in fmn (doesn't look like it either, but I might have easily missed something here as well).

praiskup · 2024-01-17T08:00:03Z

@knurd I don't think the submitter field is changed by this patch, so there is no change.

But you are right I think. The problem is the agent_name, which is not doing any translation, and doing it probably isn't easy. Since agent_name == submitter now, this could cause issues I bet. WDYT @abompard ?

abompard · 2024-01-17T08:35:20Z

Ah yes, I did not know that the submitter would be a namespaced username.
I could set it only when it's not namespaced, but the message schema shouldn't make the FASJSON call to translate the namespaced usernames to FAS usernames, that should be done by the application itself. Do you think Copr could do that @praiskup ? The call would be something like curl --negotiate -u : https://fasjson.fedoraproject.org/v1/search/users/?github_username=abompard for my Github account, for example.

praiskup · 2024-01-17T09:23:05Z

Whohoo! This fassjson thingy is awesome, seems like usable for #2977!

I could set it only when it's not namespaced,

If not namespaced, then it should already be a FAS account, sounds like a good idea to me.

but the message schema shouldn't make the FASJSON call to translate the namespaced usernames to FAS usernames

The curl call takes several tens of seconds (~45s). At some point, copr should do this translation call I think - but things need to be cached somehow, I don't know. This is again a topic for separate RFE issue I'm afraid.

praiskup · 2024-01-17T09:31:49Z

If not namespaced, then it should already be a FAS account, sounds like a good idea to me.

This is of course truth only for the Fedora Copr deployment, not other Copr deployments.

abompard · 2024-01-17T09:54:36Z

The curl call takes several tens of seconds (~45s). At some point, copr should do this translation call I think - but things need to be cached somehow, I don't know. This is again a topic for separate RFE issue I'm afraid.

It really shouldn't be so long, I need to check that. It's supposed to be an indexed LDAP call, so it should be very fast. But I'd appreciate if it were cached nonetheless ;-)

Please note that I just realized that gitlab usernames can't be queried yet. It should be very easy to add though, only a couple lines.

knurd · 2024-01-17T10:19:48Z

Thx to both of you for looking into/working on this, much appreciated. 👏

While at it a comment from the cheap seats that likely is unnecessary at this point: if looking up the github/gitlab names is to expensive/complicatd, I wonder if all of that could be avoided by sending the mails to the copr user that set up the "project package" in copr that acts when the webhook fires.

abompard · 2024-01-17T12:00:05Z

While at it a comment from the cheap seats that likely is unnecessary at this point: if looking up the github/gitlab names is to expensive/complicatd, I wonder if all of that could be avoided by sending the mails to the copr user that set up the "project package" in copr that acts when the webhook fires.

Is this user available somewhere in the data dict that is used for the body of the message? If so, then yeah that'd be the best solution IMHO.

Signed-off-by: Aurélien Bompard <[email protected]>

abompard · 2024-01-17T12:17:58Z

OK I've added a commit that should teach the schemas to ignore namespaced usernames in submitter and thus agent_name and usernames.

praiskup · 2024-01-17T15:12:33Z

Is this user available somewhere in the data dict that is used for the body of the message?

These are all good requests :-) like the other in this PR before; we need separate issue I bet, or even PR.
We don't track who creates the package in particular copr (can be anyone with admin/builder access). The token is either per-user (typically not used by automation) or for webhooks per-project (and there's no per-user ownership of those). Perhaps we speak
about #1435 here.

praiskup · 2024-01-17T15:14:59Z

The PR now LGTM, thank you @abompard

knurd · 2024-03-01T10:18:02Z

Ah yes, I did not know that the submitter would be a namespaced username.

That aspect was not fixed, wasn't it? If so: is it on some todo list, or do we need a ticket to ensure that does not fall through the cracks?

knurd · 2024-03-01T10:18:57Z

Ohh, and btw, I still don't get notifications from copr for non-namespaces things I did. Is the new code maybe not yet deployed?

praiskup · 2024-03-02T16:05:46Z

If you want to track that, submit an issue, yes. And the release/deployment is probably going to happen on Thursday 8th of March.

knurd · 2024-03-06T09:35:19Z

If you want to track that, submit an issue, yes

Thx, now done, see #3176

github-advanced-security bot found potential problems Jan 11, 2024

View reviewed changes

praiskup reviewed Jan 11, 2024

View reviewed changes

messaging/copr_messaging/private/hierarchy.py Outdated Show resolved Hide resolved

abompard force-pushed the notifications branch 2 times, most recently from 44394a5 to 7b2f173 Compare January 12, 2024 05:24

praiskup reviewed Jan 12, 2024

View reviewed changes

praiskup mentioned this pull request Jan 15, 2024

add additional properties to copr messaging schema #2486

Open

Messaging: Set the agent_name and usernames properties

5b43e44

This sets a couple properties that are standard in Fedora Messaging schemas and will allow notifications to be emitted by FMN. Signed-off-by: Aurélien Bompard <[email protected]>

abompard force-pushed the notifications branch from 7b2f173 to 5b43e44 Compare January 15, 2024 18:09

praiskup approved these changes Jan 16, 2024

View reviewed changes

Handle namespaced usernames

6e4c8ce

Signed-off-by: Aurélien Bompard <[email protected]>

praiskup merged commit b88b7bd into fedora-copr:main Jan 18, 2024
7 checks passed

knurd mentioned this pull request Mar 6, 2024

No FMN notifications when the submitter is a namespaced username #3176

Open

abompard deleted the notifications branch March 11, 2024 07:39

knurd mentioned this pull request Mar 11, 2024

No notifications from copr fedora-infra/fmn#1107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Messaging: Set the agent_name, usernames and groups properties #3099

Messaging: Set the agent_name, usernames and groups properties #3099

abompard commented Jan 11, 2024

praiskup Jan 12, 2024

praiskup Jan 12, 2024

abompard Jan 12, 2024

praiskup Jan 15, 2024

praiskup Jan 15, 2024

abompard Jan 15, 2024

abompard Jan 15, 2024

praiskup commented Jan 12, 2024 •

edited

Loading

abompard commented Jan 12, 2024

praiskup left a comment

knurd commented Jan 16, 2024 •

edited

Loading

praiskup commented Jan 17, 2024

abompard commented Jan 17, 2024

praiskup commented Jan 17, 2024

praiskup commented Jan 17, 2024

abompard commented Jan 17, 2024

knurd commented Jan 17, 2024

abompard commented Jan 17, 2024

abompard commented Jan 17, 2024

praiskup commented Jan 17, 2024 •

edited

Loading

praiskup commented Jan 17, 2024

knurd commented Mar 1, 2024

knurd commented Mar 1, 2024

praiskup commented Mar 2, 2024

knurd commented Mar 6, 2024

Messaging: Set the agent_name, usernames and groups properties #3099

Messaging: Set the agent_name, usernames and groups properties #3099

Conversation

abompard commented Jan 11, 2024

praiskup Jan 12, 2024

Choose a reason for hiding this comment

praiskup Jan 12, 2024

Choose a reason for hiding this comment

abompard Jan 12, 2024

Choose a reason for hiding this comment

praiskup Jan 15, 2024

Choose a reason for hiding this comment

praiskup Jan 15, 2024

Choose a reason for hiding this comment

abompard Jan 15, 2024

Choose a reason for hiding this comment

abompard Jan 15, 2024

Choose a reason for hiding this comment

praiskup commented Jan 12, 2024 • edited Loading

abompard commented Jan 12, 2024

praiskup left a comment

Choose a reason for hiding this comment

knurd commented Jan 16, 2024 • edited Loading

praiskup commented Jan 17, 2024

abompard commented Jan 17, 2024

praiskup commented Jan 17, 2024

praiskup commented Jan 17, 2024

abompard commented Jan 17, 2024

knurd commented Jan 17, 2024

abompard commented Jan 17, 2024

abompard commented Jan 17, 2024

praiskup commented Jan 17, 2024 • edited Loading

praiskup commented Jan 17, 2024

knurd commented Mar 1, 2024

knurd commented Mar 1, 2024

praiskup commented Mar 2, 2024

knurd commented Mar 6, 2024

praiskup commented Jan 12, 2024 •

edited

Loading

knurd commented Jan 16, 2024 •

edited

Loading

praiskup commented Jan 17, 2024 •

edited

Loading