-
Notifications
You must be signed in to change notification settings - Fork 792
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
[repo] Package validation tweaks deux #4927
[repo] Package validation tweaks deux #4927
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4927 +/- ##
==========================================
- Coverage 83.51% 83.31% -0.21%
==========================================
Files 295 295
Lines 12325 12325
==========================================
- Hits 10293 10268 -25
- Misses 2032 2057 +25
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -4,6 +4,7 @@ | |||
<PropertyGroup> | |||
<CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)/OpenTelemetry.prod.ruleset</CodeAnalysisRuleSet> | |||
<ExposeExperimentalFeatures Condition="'$(ExposeExperimentalFeatures)' == ''">true</ExposeExperimentalFeatures> | |||
<EnablePackageValidation Condition="'$(EnablePackageValidation)' == ''">false</EnablePackageValidation> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: package validation is an opt-in feature. No need to specify it here. Or am I missing something?
<EnablePackageValidation Condition="'$(EnablePackageValidation)' == ''">false</EnablePackageValidation> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a convenience thing so that when someone wants to test package validation from IDE we can toggle it to true and then kick off a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Is there any reason why you don't want to enable it by default? That's what we usually recommend. Currently, it runs part of a single CI leg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel passionately it shouldn't be ON by default. My thinking was, we have the public api analyzer currently. That's what we all suffer through at dev-time 😄 The CI also runs a pass through the package validation. If we turn this on by default, I don't think it will add much benefit. Because I don't think most devs run a pack ever. I think package validation only fires during pack? Basically it will just add some cycles at restore to fetch the stable versions which didn't seem crucial or really beneficial so I left it OFF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fetching a baseline package during restore should be super fast and not be noticeable (especially as it's cached after the first restore) but I understand where you're coming from. Thanks for the clarification :)
Follow-up to #4767
Follow-up to #4923
Changes
Microsoft.DotNet.ApiCompat
package definition and special NuGet sourceEnablePackageValidation
tofalse
by defaultPackageValidationBaselineVersion
to a target (see [repo] Package validation tweaks #4923 (comment))