Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group subscription updates #4422

Merged
merged 6 commits into from
Feb 10, 2025

Conversation

dkurepa
Copy link
Member

@dkurepa dkurepa commented Feb 5, 2025

#4336
This PR makes the service group dependency updates by keeping track of the commit that was in the latest subscription trigger.
For example, let's say we trigger a subscription (we'll call it trigger 1) that has an already existing PR, but that PR can't be updated. We set a reminder to try again later. Now let's say we trigger the subscription again (trigger 2), again, but the PR is still not updatable.
This PR makes it so that the update caused by the trigger 1 doesn't get processed, because a newer update from trigger 2 should be processed next

@dkurepa dkurepa marked this pull request as ready for review February 5, 2025 16:25
@dkurepa dkurepa requested review from premun and adamzip February 5, 2025 16:25
Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... Somehow I always assumed that we need to store the whole update so that we can compare the build times?
Isn't storing just the SHA not enough? I think we should be applying the latest build.

We should even compare probably that we don't have a newer in the PR already.

@dkurepa
Copy link
Member Author

dkurepa commented Feb 5, 2025

Hm... Somehow I always assumed that we need to store the whole update so that we can compare the build times? Isn't storing just the SHA not enough? I think we should be applying the latest build.

We should even compare probably that we don't have a newer in the PR already.

We could compare build times, but I think this might be enough.
This is because in 99% of cases, when we trigger a subscription we're flowing the latest build. So by making this nextCommitToUpdate only updatable during subscription triggers, I think we mostly cover this latest build case

@dkurepa
Copy link
Member Author

dkurepa commented Feb 5, 2025

@mmitche do you have any opinion on this? Is it ok to say that the latest build is the one that last triggered the subscription? From my experience that is the case in almost every scenario.
Also if someone triggers a subscription with a build that's not the latest, it could get ignored if the build is older than the one currently tracked, even if that is exactly what the person triggering the subscription wanted to do

@mmitche
Copy link
Member

mmitche commented Feb 5, 2025

@dkurepa In what cases can a subscription be triggered where it would not apply the latest build?

@dkurepa
Copy link
Member Author

dkurepa commented Feb 5, 2025

@dkurepa In what cases can a subscription be triggered where it would not apply the latest build?

only if someone did it in manually through darc trigger-subscriptions and provided a buildId that wasn't the latest. But I don't think anyone is doing that, at least not on any branch that we have automation set up for

@mmitche
Copy link
Member

mmitche commented Feb 5, 2025

only if someone did it in manually through darc trigger-subscriptions and provided a buildId that wasn't the latest. But I don't think anyone is doing that, at least not on any branch that we have automation set up for

Yeah, I don't think anyone is doing that. And anyway, if a new build came in, that would just get overwritten anyway.

@dkurepa dkurepa merged commit 5ffc662 into dotnet:main Feb 10, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants