Skip to content

Regular subscriptions to VMR end up pulling eng/common under /src/arcade in regular repos #4751

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

Closed
premun opened this issue Apr 28, 2025 · 8 comments
Assignees

Comments

@premun
Copy link
Member

premun commented Apr 28, 2025

Pull request

dotnet/cecil#310

Type of problem

Unexpected behavior in the PR (e.g. conflicts, EOLs, mangled files, ..)

Description of the issue

The files should go to /eng/common instead of /src/arcade/eng/common

@premun premun self-assigned this Apr 28, 2025
premun added a commit that referenced this issue Apr 28, 2025
)

- Allows to use the `flow-commit` command for any repos (VMR and
non-VMR).
- Allows to add a list of packages to be flown too

#4751
premun added a commit to premun/arcade-services that referenced this issue Apr 28, 2025
@premun premun removed their assignment Apr 29, 2025
@premun
Copy link
Member Author

premun commented Apr 29, 2025

Originally, I thought this happened here:

IDependencyFileManager arcadeFileManager = await remoteFactory.CreateDependencyFileManagerAsync(arcadeItem.RepoUri);
try
{
targetDotNetVersion = await arcadeFileManager.ReadToolsDotnetVersionAsync(arcadeItem.RepoUri, arcadeItem.Commit, sourceRepoIsVmr);
}
catch (DependencyFileNotFoundException)
{
// global.json not found in src/arcade meaning that repo is not the VMR
sourceRepoIsVmr = false;
targetDotNetVersion = await arcadeFileManager.ReadToolsDotnetVersionAsync(arcadeItem.RepoUri, arcadeItem.Commit, sourceRepoIsVmr);
}

but we do not throw DependencyFileNotFoundException when rate limiting happens:

catch (HttpRequestException reqEx) when (reqEx.Message.Contains(((int)HttpStatusCode.NotFound).ToString()))
{
throw new DependencyFileNotFoundException(filePath, $"{owner}/{repo}", branch, reqEx);
}

@premun premun self-assigned this Apr 29, 2025
@premun
Copy link
Member Author

premun commented Apr 29, 2025

This one does not reproduce anymore and I was not able to figure out how it happened in the first place even. Let's close it and re-open if we see it again

@premun premun closed this as completed Apr 29, 2025
@akoeplinger
Copy link
Member

For the record, it happened in dotnet/emsdk#1192 as well. Closing the PR and retriggering the subscription didn't reproduce it though.

@premun premun reopened this May 6, 2025
@premun
Copy link
Member Author

premun commented May 6, 2025

Thanks for letting us know. I suspect that it might be that when we're verifying existence of some files to tell a repo from a VMR, the request fails for other reasons (e.g. rate limiting) and we cannot distinguish that and then wrongly consider the source repo not a VMR. Reopening

@premun premun moved this to Ready in .NET Unified Build May 7, 2025
@premun premun removed their assignment May 7, 2025
premun added a commit that referenced this issue May 7, 2025
A possible fix for #4751
where we use `NotFound` as a signal whether a source repo is or is not a
VMR
@premun premun self-assigned this May 9, 2025
@wtgodbe
Copy link
Member

wtgodbe commented May 12, 2025

Happened in dotnet/aspnetcore#61878 too

@dkurepa dkurepa self-assigned this May 13, 2025
@dkurepa dkurepa moved this from Ready to In Progress in .NET Unified Build May 13, 2025
@akoeplinger
Copy link
Member

looks like a bunch of repos accidentally merged the wrong src/arcade changes: https://github.com/search?q=org%3Adotnet%20path%3Asrc%2Farcade%2Feng%2Fcommon%2FREADME.md&type=code

@dkurepa
Copy link
Member

dkurepa commented May 14, 2025

ouch, looks like we're going to have to remove all of those, don't think they'll be gone after we fix this. Thanks for bringing it up

michalpavelka pushed a commit that referenced this issue May 14, 2025
<!-- Link the GitHub or AzDO issue this pull request is associated with.
Please copy and paste the full URL rather than using the
dotnet/arcade-services# syntax -->
#4751

---------

Co-authored-by: Copilot <[email protected]>
@dkurepa
Copy link
Member

dkurepa commented May 15, 2025

we rolled out the cashing fix last night, this shouldn't happen anymore. We'll now need to cleanup the repos that got the src/arcade/eng/common PRs merged

Tracking issue for the cleanup: #4837

@dkurepa dkurepa closed this as completed May 15, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in .NET Unified Build May 15, 2025
akoeplinger added a commit to akoeplinger/try that referenced this issue May 15, 2025
It was accidentally added due to a bug in darc: dotnet/arcade-services#4751
jonsequitur pushed a commit to dotnet/try that referenced this issue May 15, 2025
It was accidentally added due to a bug in darc: dotnet/arcade-services#4751
@premun premun marked this as a duplicate of #4830 May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

4 participants