Skip to content

Remove the need for a global lock when building or publishing a project #21753

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

Merged
merged 1 commit into from
May 13, 2020

Conversation

pranavkm
Copy link
Contributor

  • Change template test build and publish to not perform dotnet restore
  • Remove locking requirements for build and publish
  • Increase timeout for dotnet new operations since it's network bound

* Change template test build and publish to not perform dotnet restore
* Remove locking requirements for build and publish
* Increase timeout for dotnet new operations since it's network bound
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 12, 2020
@pranavkm pranavkm marked this pull request as ready for review May 12, 2020 22:15
@pranavkm pranavkm requested a review from NTaylorMullen May 12, 2020 22:24
@NTaylorMullen
Copy link

If you feel this does in fact fix the reliance on the dotnet new lock feel free to revert: 9a5d3c7

@pranavkm
Copy link
Contributor Author

If you feel this does in fact fix the reliance on the dotnet new lock feel free to revert

While I think it should be fixed, we'll let DOI make that call.

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

This seems fine except for the very-long timeout. Probably wanna re-run it a couple times just to be sure nothing falls out.

@@ -24,7 +24,7 @@ public ProcessLock(string name)

public async Task WaitAsync(TimeSpan? timeout = null)
{
timeout ??= TimeSpan.FromMinutes(2);
timeout ??= TimeSpan.FromMinutes(20);
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems excessive...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but these used to wait indefinitely, so it's a minor improvement over that.

Copy link
Contributor

Choose a reason for hiding this comment

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

What goes wrong if they only wait 2 minutes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I added these timeouts here, the build passed a few times successfully on public AzDo, and promptly failed in the internal Azdo. This of course included the time it takes to build \ publish which is being changed here. What I'm going with here is that it's hard to pick a reasonable time, but we also do not want tests to fail just because restore took too long that one time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants