-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix the official build #3020
Fix the official build #3020
Conversation
80beb3b
to
bf1e431
Compare
@@ -4,8 +4,5 @@ | |||
<packageSources> | |||
<clear/> | |||
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" /> | |||
<add key="AzureFunctionsTempStaging" value="https://pkgs.dev.azure.com/azfunc/e6a70c92-4128-439f-8012-382fe78d6396/_packaging/AzureFunctionsTempStaging/nuget/v3/index.json" /> | |||
<add key="durabletask" value="https://pkgs.dev.azure.com/durabletaskframework/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask/nuget/v3/index.json" /> | |||
<add key="durabletask-test" value="https://durabletaskframework.pkgs.visualstudio.com/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask-test/nuget/v3/index.json" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change because it makes the nuget restore behavior deterministic when the same version exists in multiple feeds, but I just want to check with @bachuv and @nytian that this change is safe to make. In particular, I'm wondering whether it changes how we need to go about our release process.
If we need to keep relying on some of these feeds, we could look into cleaning up these alternate feeds to make sure we don't have any duplicate packages across them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with removing these package sources.
add key="AzureFunctionsTempStaging" value="https://pkgs.dev.azure.com/azfunc/e6a70c92-4128-439f-8012-382fe78d6396/_packaging/AzureFunctionsTempStaging/nuget/v3/index.json"
I'm actually not sure why we have this package source. Maybe @nytian knows.
add key="durabletask" value="https://pkgs.dev.azure.com/durabletaskframework/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask/nuget/v3/index.json" />
add key="durabletask-test" value="https://durabletaskframework.pkgs.visualstudio.com/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask-test/nuget/v3/index.json" />
These package sources are from our old ADO durabletaskframework org which we don't use anymore so we should remove these.
We can consider adding our new azfunc
artifact feed to the package sources: add key="durabletask-test" value="https://azfunc.pkgs.visualstudio.com/internal/_packaging/durabletask-test/nuget/v3/index.json"
, but we can also add this in the future if it's helpful.
Overall, I only use package sources that are not nuget.org when I'm performing local tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure why we have this package source. Maybe @nytian knows.
Actually, I may be able to answer this, although I'll let Naiyuan confirm
The smoke tests were relying on a prerelease package of the .NET worker SDK for some behavior fix, version 1.16.0-preview2. I assume that we brought in the staging source to get this package. This PR updates to a released version that is on nuget.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply! Actually, I am not quite sure about what this is, and I thought nuget.org is enough for downloading all the packages. I didn't recall any package downloaded from this source at VS, but I might be wrong :)@andystaples please let us know if you have context about this, thank you!
@@ -134,7 +134,7 @@ private void ThrowIfPlatformLevelException(FailureDetails failureDetails) | |||
// It's unclear if all OOMs are caught by the isolated process (probably not), and also if there are other platform-level | |||
// errors that are also caught in the isolated process and returned as a `OrchestratorExecutionResult`. Let's add them | |||
// to this method as we encounter them. | |||
if (failureDetails.InnerFailure?.IsCausedBy<OutOfMemoryException>() ?? false) | |||
if (failureDetails.IsCausedBy<OutOfMemoryException>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, which test failed before you made this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The smoke test for Out of Memory exception retry handling - when David and I were writing that feature, every case we saw had the OOM wrapped at least one layer of InnerFailure deep, so we skipped checking the outermost failure details. Turns out, (likely when I upgraded the worker SDK version) this behavior changed and OOM became the outermost failure, meaning our check didn't catch it. Likely why we've had customers saying that the fix didn't help their app - because they were on latest worker SDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! just left a small question
@@ -15,7 +15,7 @@ on: | |||
|
|||
jobs: | |||
build: | |||
runs-on: ubuntu-latest | |||
runs-on: ubuntu-22.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm, this change is because the ubuntu-latest image in Github is having issue, so we have to pick a version for now? I am asking this because I am wondering if we need to change this back in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - we should change this back in the future, once the bug is fixed in ubuntu-latest
Issue describing the changes in this PR
Fixes several pipeline issues:
Pull request checklist
pending_docs.md
release_notes.md
/src/Worker.Extensions.DurableTask/AssemblyInfo.cs
dev
andmain
branches and will not be merged into thev2.x
branch.