Do we really want to be firing one hook per branch update? It seems to me that once we exceed some threshold (e.g. 10 branch or tag pushes), we should just aggregate the push data into a single field and send one event per hook/service. The activity feed also shouldn't be littered with 10,000 push events; maybe 1 post should be adequate there too.
Expected outcome
Don't fire hooks/services if there are more than 3 branches/tags in a single push.
This was spawned from a gitlab-ce3857370 gitlab-ce3713903 issue, https://gitlab.com/gitlab-org/gitlab-ce/issues/65804, setting the priority to gitlab-ce3857523 gitlab-ce3713902 initially but feel free to adjust
@stanhu just want to clarify... are we planning to aggregate all the push data in one event per hook/service or are we going to not fire a hook/service if the push size reaches a certain threshold (number of branches/tags)?
I'm skeptical about the first one (aggregating) as it feels like it's just going to be the same. If we aggregate, we may end up with a big push data that we'll need to split into multiple jobs which seems to be already happening now. I don't think we can push the aggregated push data to a webhook/service as they usually expect a single tag/branch per event (I could be wrong though).
I'm skeptical about the first one (aggregating) as it feels like it's just going to be the same. If we aggregate, we may end up with a big push data that we'll need to split into multiple jobs which seems to be already happening now. I don't think we can push the aggregated push data to a webhook/service as they usually expect a single tag/branch per event (I could be wrong though).
@patrickbajao Yeah, this is a product question. If someone pushes 1,000 tags, I think it would be nice to say User X pushed N tags in the activity feed and leave it at that, the same way we omit all the commits when someone rebases from master. The alternative is to skip creating an event altogether, which seems like it could lead to confusion.
For the push hooks, I personally think it's fine if we just skip firing a hook if there are too many tags/branches.
@stanhu Yup, that's what I think too. But just wanted to clarify your thoughts first since you suggested this limit.
If someone pushes 1,000 tags, I think it would be nice to say User X pushed N tags in the activity feed and leave it at that, the same way we omit all the commits when someone rebases from master.
This is going to be worked on #31007 (closed) so I'll keep the discussion in that issue.
For the push hooks, I personally think it's fine if we just skip firing a hook if there are too many tags/branches.
Yup, same here. The question now is what's the right threshold. GH uses 3 but I don't know how they came up with that number. I feel this number should be the same with the limit we have for activity feed for a start. That way there is some sort of consistency. We can tweak it (lower or higher) later if needed.
WDYT? @jramsay tagging you here as well. Please share what you think about this.
The question now is what's the right threshold. GH uses 3 but I don't know how they came up with that number. I feel this number should be the same with the limit we have for activity feed for a start. That way there is some sort of consistency. We can tweak it (lower or higher) later if needed.
@patrickbajao I agree the limits for activity feed and hooks should be the same. I also think it is better to start low and then increase if there is a strong reason. I also don't have any magic reasoning for what number we should start with, but 3 seems as good a number as any
@stanhu@reprazent Just like #31007 (closed), this is fixed, documented and the MRs are public, so can we make this issue non-confidential? We link to it from the docs, as is our requirement.