Skip to content

Get SDK resolver interactivity level from SdkResolverContext#2390

Merged
jainaashish merged 2 commits into
NuGet:devfrom
jeffkl:interactive
Aug 22, 2018
Merged

Get SDK resolver interactivity level from SdkResolverContext#2390
jainaashish merged 2 commits into
NuGet:devfrom
jeffkl:interactive

Conversation

@jeffkl
Copy link
Copy Markdown
Contributor

@jeffkl jeffkl commented Aug 16, 2018

Add an extension method that uses reflection to read the new property

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented Aug 16, 2018

/cc @Microsoft/msbuild-maintainers

@jeffkl jeffkl changed the base branch from dev to release-4.9.0-preview2 August 17, 2018 16:36
@jeffkl jeffkl changed the base branch from release-4.9.0-preview2 to dev August 17, 2018 16:36
@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented Aug 17, 2018

Okay I've updated to use our preview package, please review

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented Aug 17, 2018

Another option would be to use reflection so I don't need to update the target framework and use a preview version of our package.

@jainaashish
Copy link
Copy Markdown

Not sure what's the ask of this change and which VS branch is this targeting? Is this already approved for 15.9? Do we also need this for dev16? And who'd make sure to remove dotnet-msbuild feed and move to a stable Microsoft.Build.Framework package?

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented Aug 20, 2018

At the moment the credential plug-in will not prompt for login because we've hard coded the call to SetupDefaultCredentialService with nonInteractive: true. In 15.9, I added some logic in MSBuild to detect the interactivity level that the user specified and piped that through to the SDK resolver.

To take the change, I either need to compile against a newer version of the Microsoft.Build.Framework package which is the reason we need to use our other package feed. Another option would be to just use reflection. That would not require me to use a newer package or alternative feed. I'm open to either one.

Yes this can get approved for 15.9 and the MSBuild change is already in.

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented Aug 21, 2018

@jainaashish I've changed this to use reflection instead so its not touching as much. Please let me know when this can get into 15.9 as we need it ASAP to continue testing the authenticated feed scenarios

Copy link
Copy Markdown

@jainaashish jainaashish left a comment

Choose a reason for hiding this comment

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

LGTM

@jainaashish
Copy link
Copy Markdown

as I said on another thread, we first need to merge it to dev and then release-4.9.0-preview2.

Once we've substantial changes to be inserted into 15.9, we'll start with our insertion process (hopefully tomorrow or day after).

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented Aug 21, 2018

Thanks, please merge when the CI completes

@jainaashish
Copy link
Copy Markdown

Test signing failed for the last build. CI Build triggered again

@jainaashish jainaashish merged commit 62916a4 into NuGet:dev Aug 22, 2018
jainaashish pushed a commit that referenced this pull request Aug 22, 2018
* Use reflection via an extension method to get the Interactive property

* Improve readability
@jainaashish
Copy link
Copy Markdown

Merged into 4.9.0 with 814cc67

@jeffkl jeffkl deleted the interactive branch August 22, 2018 16:10
@nkolev92
Copy link
Copy Markdown
Member

What's the Visual Studio default?
Would a user need to set this interactive switch in the project to get it to prompt in VS?

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented Aug 28, 2018

The default is non-interactive. In Visual Studio, we're using a different credential provider at the moment. In dev16, the only way to set this property will be through a command-line argument to ensure its only set in a console window.

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.

5 participants