Skip to content

Logging site extension is missing net5.0 runtime store #34862

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.

Sign up for GitHub

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

Jump to bottom
Merged
shirhatti merged 4 commits into from
Jul 30, 2021
Merged

Logging site extension is missing net5.0 runtime store #34862

shirhatti merged 4 commits into from
Jul 30, 2021

Conversation

@shirhatti
Copy link
Contributor

@shirhatti shirhatti commented Jul 29, 2021

No description provided.

@shirhatti shirhatti requested a review from Pilchie as a code owner July 29, 2021 23:56
shirhatti
shirhatti commented Jul 29, 2021
View reviewed changes
dougbu
dougbu reviewed Jul 30, 2021
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

I'll approve once your testing is done, eng/Versions.props is restored, and you've undone your TODO bit

...re.AzureAppServices.SiteExtension/Microsoft.AspNetCore.AzureAppServices.SiteExtension.csproj
Coming soon...
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.5.0.x64" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.5.0.x86" PrivateAssets="All" />
-->
Copy link
Contributor

@dougbu dougbu Jul 30, 2021

Choose a reason for hiding this comment

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

Good catch ❕

@shirhatti
Copy link
Contributor Author

shirhatti commented Jul 30, 2021

I just realized that the SiteExtension build step is skipped for PRs. @dotnet/aspnet-build how would I verify this change?

@dougbu
Copy link
Contributor

dougbu commented Jul 30, 2021

@dotnet/aspnet-build how would I verify this change?

Either

  1. Add another temporary change to remove the condition: in .azure/pipelines/ci.yml for the 'Build SiteExtension' job
  2. Push your branch to the dotnet-aspnetcore AzDO (mirror) repo and manually start a build there; don't create an internal PR

@shirhatti shirhatti requested a review from a team as a code owner July 30, 2021 00:26
dougbu
dougbu approved these changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Looks great if I squint and ignore the TODO items that you'll remove. Thanks again

@dougbu
Copy link
Contributor

dougbu commented Jul 30, 2021

@JunTaoLuo are there current issues w/ Components.E2ETests and the SignalR tests❔ I can't see how @shirhatti's changes would cause the failures in the current build

@BrennanConroy
Copy link
Member

BrennanConroy commented Jul 30, 2021

SignalR tests

Hmm, just added that test, I'll try to see if it's flaky and fix it

@shirhatti
Copy link
Contributor Author

shirhatti commented Jul 30, 2021

Verified that the right package got built
image

@shirhatti shirhatti enabled auto-merge (squash) July 30, 2021 07:17
@BrennanConroy
Copy link
Member

BrennanConroy commented Jul 30, 2021

Am I confusing issues? In the email you said we didn't have the 6.0 folder with a picture, but this adds the 5.0 folder which was in the picture.

@shirhatti
Copy link
Contributor Author

shirhatti commented Jul 30, 2021

Am I confusing issues? In the email you said we didn't have the 6.0 folder with a picture, but this adds the 5.0 folder which was in the picture.

I misspoke in the email. I was looking at the 5.0 package (since we don't publish SiteExtension packages for preview releases). Turns out I found another issue in the process 😅

@BrennanConroy
Copy link
Member

BrennanConroy commented Jul 30, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 30, 2021

Azure Pipelines successfully started running 2 pipeline(s).

@shirhatti shirhatti merged commit 11dd487 into main Jul 30, 2021
@shirhatti shirhatti deleted the shirhatti/logging-site-extension branch July 30, 2021 19:50
@ghost ghost added this to the 6.0-rc1 milestone Jul 30, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pilchie Pilchie Pilchie approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

+1 more reviewer

@dougbu dougbu dougbu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

6.0-rc1

Development

Successfully merging this pull request may close these issues.

6 participants

@shirhatti @dougbu @BrennanConroy @Pilchie @amcasey