This surely is unexpected behavior, I will have a look at it!

Ok, I see it.
The toddler was originally made to remove users that are not packagers anymore on distgit side. Because there is (or we thought there is) a sync between IPA groups and distgit groups, the toddler also removes the user on the IPA side, because if not, the user would get synced back from IPA to distgit.
This is an unexpected situation which the toddler was not supposed to handle: The user at the same time:
- is not a packager
- is not in distgit group crypto-team
- is in IPA group crypto-team

The question is, why the user does not get synced from IPA group to distgit group?
The sync is something the toddler is expecting to happen and then would deal with (by removing user from both distgit and IPA). Since the distgit state of things is in desired state, the toddler does not do anything.

I would see two ways to go about this:
1. Investigate why the sync from IPA to distgit does not happen (should it? or is the sync limited only to certain groups and crypto-team in not one of them?)
2. extend the toddler scope to also handle situations like this

Thoughts?
@abompard @kevin @zlopez @gotmax23

Answering this based on my understanding — anyone feel free to correct me if I'm wrong :)

@lenkaseg said:

This is an unexpected situation which the toddler was not supposed to handle:

The question is, why the user does not get synced from IPA group to distgit group?

@gotmax23 said:

As far as I know, group membership only get synced to distgit when the respective users log in/log out so the list of group members provided in the distgit API response is likely incomplete.

In other words, I don't think there's any other syncing of users from IPA to distgit outside of user logins. So a user can be a member of the group in IPA and if they log into distgit, their memberships would sync and they would be given access to all the crypto-team packages (or whatever other groups they belong to), even if they currently aren't listed in the group on distgit.

@lenkaseg said:

Because there is (or we thought there is) a sync between IPA groups and distgit groups, the toddler also removes the user on the IPA side, because if not, the user would get synced back from IPA to distgit.

Indeed, as you said, the (non-packager) user needs to be removed from both the distgit group and IPA group if the user is a member of the corresponding groups on both sides so that the user isn't readded if they log in/log out again. But since it's possible for the user to not have recently logged in to sync the groups and only be a member of the group on the IPA side, the toddler also needs to handle the case where a user only exists in the IPA group and thus only needs to be removed from there.

Right, the sync is only on login.

Ok, let's add the purge on the IPA side.

So the playtime call of the toddler will check the list of users in IPA packagers, then loop through all the distgit groups and remove any user who is not on the list.
When the toddler identifies a user to be removed from distgit group, it automatically removes them from IPA side as well. (this won't be necessary, because =>)
Then we have to loop through IPA groups that have distgit equivalent and remove any user who is not on the IPA packager list.

We discussed this issue with @abompard and it resulted in opening following issue on nogging: https://github.com/fedora-infra/noggin/issues/1563
There should be a warning when a sponsor adds a user who is not a packager, into an IPA packaging group.

+1 to both comments. Thanks @lenkaseg!

After discussing this issue further with @abompard, this seems more like a separate functionality that might deserve a separate toddler.

We would need to define all packaging groups in IPA (are they exactly aligned with distgit groups minus sysadmin-main and cvsadmin or can there be more packaging groups on IPA that don't have distgit equivalent?)
Maybe would be a good idea to tag the packager groups in the database with an argument something like "packager_group" to be able to gather all of them in one call.

A fast fix to handle most cases would be to add a function that loops through all IPA packaging groups (defined as groups that have a distgit equivalent minus sysadmin-main and cvsadmin) and removes all users that are not IPA packager members.

all packaging groups are in ipa, but there's.... vastly more non packaging groups in there too. I don't think there's currently a way to tell if a group is a packaging group or not just from IPA. You need to look at the pagure side and see if the group exists there. If it does, its a packaging group (or some other group pagure uses like sysadmin-main / cvsadmin).
I'm not sure how to tag groups. I see there's a indicator for if something is a 'fas group' already, but thats just hiding ipa groups we don't want to expose from fas. Perhaps abompard knows a way to tag them? Or... we could make a new group 'packaing-groups' and add all those groups to that group?

A fast fix to handle most cases would be to add a function that loops through all IPA packaging groups (defined as groups that have a distgit equivalent minus sysadmin-main and cvsadmin) and removes all users that are not IPA packager members.

Yes, that sounds right.

If the packaging groups in IPA can be defined as has a distgit equivalent minus (sysadmin-main and cvsadmin) then it should be easy.

I think @abompard mentioned there is a way to tag them/add a queryable argument to them in the database. Only now with the migration to the new src forge, some rules/structure might be revisioned. Might be better to wait until things settle with the src forge.

What's the status here?