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

Warn if the publish profile provided is not present in the project #30239

Merged
merged 3 commits into from Feb 2, 2023

Conversation

vijayrkn
Copy link
Contributor

@vijayrkn vijayrkn commented Feb 1, 2023

Fixes: #28370

Fix:
Adds a warning if the publish profile is provided but is not imported and is not present in the project.
The fix also makes sure that websdk will mark the profile as imported if the default profile is imported.

Why this fix?
With publish profiles getting more and more popular and most projects now onboarding to this profile concept, it is important that we warn the user if their provided profile is not imported for any reason.

why warning and not an error?
PublishProfile property is just an msbuild property that the sdk handles in a specific way. It would be aggressive to fail the build just because the user passed an arbitrary msbuild property. Also, having shipped this way for the last various versions, it is probably safer to warn than to error in this scenario.

Tests:
Added tests for both the success and failure scenarios.

@baronfel / @noahfalk - Let me know if you have any concerns with the approach/fix here.

@dsplaisted
Copy link
Member

why warning and not an error?
PublishProfile property is just an msbuild property that the sdk handles in a specific way. It would be aggressive to fail the build just because the user passed an arbitrary msbuild property.

If the SDK uses a property and it has an invalid value, it is appropriate to generate an error and fail the build. We do this for all sorts of other MSBuild properties.

Also, having shipped this way for the last various versions, it is probably safer to warn than to error in this scenario.

This could be a reason to generate a warning instead of an error. There's probably no way to tell how many people are passing invalid publish profiles, so it could be safer to generate a warning. On the other hand, warnings still can generate build breaks for folks that have warnings as errors enabled.

@vijayrkn vijayrkn marked this pull request as draft February 1, 2023 17:32
@vijayrkn
Copy link
Contributor Author

vijayrkn commented Feb 1, 2023

I am marking this as draft since I have identified an issue if a Web SDK based project references a .NET SDK based class lib and the profile passed to it is a default profile, then it could generate un-intended warnings.

@baronfel
Copy link
Member

baronfel commented Feb 1, 2023

It would be good to do something here - this request has come up a couple times before, and in order to hit this case users have to be explicitly specifying a publish profile (meaning they are already customizing their builds, so they'd likely be ok with making an adjustment).

@vijayrkn
Copy link
Contributor Author

vijayrkn commented Feb 2, 2023

I am marking this as draft since I have identified an issue if a Web SDK based project references a .NET SDK based class lib and the profile passed to it is a default profile, then it could generate un-intended warnings.

The code was working as expected. The publish command is only run once for the project getting published. The reason I saw multiple warnings while testing was when I ran it at the solution level instead of project level. Multiple warnings are expected when the publish is run at the solution level for each project getting published. So, moving the PR out of draft.

Thanks @dsplaisted for the pointer. I completely missed the fact that I was unknowingly running the command on the solution instead of the project.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

I'd lean towards generating an error message, but generating a warning is better than nothing at all, and it looks like we may add telemetry around this to help us get data about whether we should later change the warning to an error.

Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

Thank you for making this, it's a big improvement in the UX 🎆

src/Tasks/Common/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Common/Resources/Strings.resx Outdated Show resolved Hide resolved
@vijayrkn vijayrkn merged commit f0a3e52 into release/7.0.3xx Feb 2, 2023
@vijayrkn vijayrkn deleted the dev/vramak/WarnForNonExistantProfiles branch February 2, 2023 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants