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

Component-based template validation PoC #5838

Merged
merged 4 commits into from
Mar 14, 2023

Conversation

vlada-shubina
Copy link
Member

@vlada-shubina vlada-shubina commented Dec 28, 2022

Problem

#2623

Solution

Component-based template validation.
Note: the design done under assumption that ITemplateValidator and ITemplateValidatorFactory becomes public later on and any host or tool can create more validator components and inject the checks.
Note: current validation checks are only those which existed before, and serve as example. More checks to be added later on.

TODO:

  • extract localization strings (once the approach is approved)

Checks:

  • Added unit tests
  • Added #nullable enable to all the modified files ?

@vlada-shubina vlada-shubina requested a review from a team as a code owner December 28, 2022 12:17
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

This is great!

IReadOnlyList<ScannedTemplateInfo> foundTemplates = Task.Run(async () => await GetTemplatesFromMountPointInternalAsync(source, default).ConfigureAwait(false)).GetAwaiter().GetResult();
localizations = foundTemplates.SelectMany(t => t.Localizations.Values).ToList();
IReadOnlyList<ScannedTemplateInfo> foundTemplates =
Task.Run(async () => await GetTemplatesFromMountPointInternalAsync(source, default).ConfigureAwait(false)).GetAwaiter().GetResult();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

this is "better than nothing" attempt to avoid deadlock (we used to have them with XUnit).
Running anything in threadpool ensures no context, therefore less chance for deadlocks.

This is based on https://learn.microsoft.com/en-us/archive/msdn-magazine/2015/july/async-programming-brownfield-async-development#the-thread-pool-hack and https://devblogs.microsoft.com/pfxteam/should-i-expose-synchronous-wrappers-for-asynchronous-methods/?WT.mc_id=DT-MVP-5000058 and so far I haven't seen better alternative.

internal enum ValidationScope
{
None = 0,
Scanning = 1,
Copy link
Member

Choose a reason for hiding this comment

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

is "Scanning" for installed templates?
what does it mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

The check is run when scanning the mount point for templates, typically installation or scanning scenario by 3rd party tools scenario. Calling it installation is somewhat wrong, as we expose API to scan templates, therefore I ended up with scanning.

I added documentation to those members.

// Localizations provide more translations than the number of manual instructions we have.
string excessInstructionLocalizationIds = string.Join(
", ",
postActionLocModel.Instructions.Keys.Where(k => !postAction.ManualInstructionInfo.Any(i => i.Id == k)));
Copy link
Member

Choose a reason for hiding this comment

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

is it possible that we have a lot of concatenations here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. This is existing code (now in

string excessInstructionLocalizationIds = string.Join(
) and let's keep it as is for now.

@YuliiaKovalova
Copy link
Member

looks good to me !

@vlada-shubina vlada-shubina merged commit 348b46c into feature/validation Mar 14, 2023
vlada-shubina added a commit that referenced this pull request Mar 15, 2023
* poc

* logging validation messages

* changes after review

* added missing localization
vlada-shubina added a commit that referenced this pull request Mar 17, 2023
* poc

* logging validation messages

* changes after review

* added missing localization
@marcpopMSFT marcpopMSFT deleted the dev/vshubina/validation-poc branch September 6, 2024 22:36
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.

3 participants