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

Update precedence of templates for 7.0 #39783

merged 3 commits into from
Jan 26, 2022

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 a team January 26, 2022 18:05
@wtgodbe wtgodbe requested review from dougbu, Pilchie and a team as code owners January 26, 2022 18:05
"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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Just created. The link should be active soon.

Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

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 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 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 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 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
Member

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 commented Jan 26, 2022

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

@halter73
Copy link
Member

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

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 26, 2022

/backport to release/7.0-preview1

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

@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) January 26, 2022 22:30
@wtgodbe wtgodbe merged commit 4fc8081 into dotnet:main Jan 26, 2022
@ghost ghost added this to the 7.0-preview1 milestone Jan 26, 2022
@wtgodbe wtgodbe deleted the wtgodbe/Precedence branch January 26, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants