The Wayback Machine - https://web.archive.org/web/20220127200022/https://github.com/dotnet/aspnetcore/pull/39783
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

Update precedence of templates for 7.0 #39783

Merged
merged 3 commits into from Jan 26, 2022
Merged

Conversation

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jan 26, 2022

Part of #39731. Going to push an update to this PR that updates documentation as well (needed to create the PR first so I could link to it from the docs)

@wtgodbe wtgodbe requested review from HaoK, javiercn and Jan 26, 2022
@wtgodbe wtgodbe requested review from dougbu, Pilchie and as code owners Jan 26, 2022
"shortName": "blazorserver",
"thirdPartyNotices": "https://aka.ms/aspnetcore/6.0-third-party-notices",
"thirdPartyNotices": "https://aka.ms/aspnetcore/7.0-third-party-notices",
Copy link
Member Author

@wtgodbe wtgodbe Jan 26, 2022

Choose a reason for hiding this comment

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

@mkArtakMSFT could you update/create this aka.ms link?

HaoK
HaoK approved these changes Jan 26, 2022
Copy link
Member

@HaoK HaoK left a comment

Changes look good, sorta of related question, is there any kind of test we can add to guard against these precedence changes being incorrect?

@wtgodbe
Copy link
Member Author

@wtgodbe wtgodbe commented Jan 26, 2022

Changes look good, sorta of related question, is there any kind of test we can add to guard against these precedence changes being incorrect?

I'm not sure, I wasn't aware of them until today. Short of waiting for an SDK that ingests them, maybe @MichalPavlik or @vlada-shubina would have an idea?

@HaoK
Copy link
Member

@HaoK HaoK commented Jan 26, 2022

Or maybe we just ask CTI to add a scenario where they have 6.0 and 7.0 installed, and run dotnet new, I believe that would catch it right since it would install the wrong version? We could automate that but that's much harder for us to do with our tests and the versioning hell than just verifying this on real builds

@wtgodbe
Copy link
Member Author

@wtgodbe wtgodbe commented Jan 26, 2022

Or maybe we just ask CTI to add a scenario where they have 6.0 and 7.0 installed, and run dotnet new, I believe that would catch it right since it would install the wrong version? We could automate that but that's much harder for us to do with our tests and the versioning hell than just verifying this on real builds

We could, but we'd still need to wait until there's an SDK that has ingested our change - I'm not sure if there's way to do it in this repo in isolation

@HaoK
Copy link
Member

@HaoK HaoK commented Jan 26, 2022

Yeah that's fine, basically we merge this as is, and then ask CTI to add a scenario for this to start testing once the changes are actually in

@vlada-shubina
Copy link

@vlada-shubina vlada-shubina commented Jan 26, 2022

Changes look good, sorta of related question, is there any kind of test we can add to guard against these precedence changes being incorrect?

I'm not sure, I wasn't aware of them until today. Short of waiting for an SDK that ingests them, maybe @MichalPavlik or @vlada-shubina would have an idea?

We have an idea of enabling template validation via MSBuild tasks: dotnet/templating#3828, however it might not meet the bar in near future. MSBuild tasks package is already in use in several repos for localization and we are onboarding others, so it's a matter of adding validation logic to them to report error/warnings in templates during the build. Detecting precedence / identity clashes is a bit tricky as in these scenarios error is over 2 or more packages, not individual templates inside same packages, but still detecting it is possible.

@dougbu
Copy link
Member

@dougbu dougbu commented Jan 26, 2022

@halter73 is there a general issue affecting acquisition of Windows build agents

@halter73
Copy link
Member

@halter73 halter73 commented Jan 26, 2022

I found another PR that was similarly affected. I retriggered the canceled builds.

@wtgodbe
Copy link
Member Author

@wtgodbe wtgodbe commented Jan 26, 2022

/backport to release/7.0-preview1

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Jan 26, 2022

Started backporting to release/7.0-preview1: https://github.com/dotnet/aspnetcore/actions/runs/1753319370

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Jan 26, 2022

@wtgodbe backporting to release/7.0-preview1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Update submodule
Using index info to reconstruct a base tree...
M	src/submodules/spa-templates
Falling back to patching base and 3-way merge...
Failed to merge submodule src/submodules/spa-templates (not checked out)
Auto-merging src/submodules/spa-templates
CONFLICT (submodule): Merge conflict in src/submodules/spa-templates
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Update submodule
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@wtgodbe wtgodbe enabled auto-merge (squash) Jan 26, 2022
@wtgodbe wtgodbe merged commit 4fc8081 into dotnet:main Jan 26, 2022
25 checks passed
@msftbot msftbot bot added this to the 7.0-preview1 milestone Jan 26, 2022
@wtgodbe wtgodbe deleted the wtgodbe/Precedence branch Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants