Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#3332 add devcontainer #3520
#3332 add devcontainer #3520
Changes from 9 commits
28164f1
d8df6c8
c455ab6
d1817a1
4ac4989
b7b47d6
eea40c8
cb5d8f5
3fe2bbd
bb0bcaf
ec596f4
803aca5
b8618b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We should just use the 8.0 SDK image here.
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.
The main differences currently between the devcontainers image and the official dotnet SDK image is that
I think those are enough of a value-add (especially the XMLDocs) to justify using this base image.
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.
Educate me -- why doesn't the SDK image get the xmldocs?
RE: Powershell -- easy add-on as a feature (which is in this as well)
No objections really either, just felt like the actual SDK image felt like a better base -- it's what I use exclusively and just add features.
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.
dotnet/dotnet-docker#2790
TLDR is "performance".
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.
Funnily enough, I was just going to the source and noticed that powershell core is already on the SDK images - so that removes one of the benefits of the devcontainers image. Kinda feels like we're all over the place with these images, thematically :)
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.
Yep, noticed that too -- this devcontainer image looks to take a patch to the powershell also though, which i would expect the powershell feature latest to have also. Anyhow, thanks for the edu on the xmldocs -- seems odd our sdk image doesn't really match the sdk, but at least now I know!
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.
It was super surprising to me too when I learned this too. I understand the constraints and get it adds a lot of value to the "non-human-interactive" use cases. So it seems when a human is involved, its best to start with the dev container image.
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.
recommend changing this to
ms-dotnettools.csdevkit
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.
are there reasons to it? had the same recommendation from @baronfel.
Isn't the intellicode extension just the more advanced one?
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.
https://marketplace.visualstudio.com/items?itemName=ms-dotnettools.vscodeintellicode-csharp
seemed we should install
ms-dotnettools.csdevkit
to keep it workingThere 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.
If you open the extension page in visual studio code you see it will install the C# extension by itself:
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.
@WeihanLi It's the same like for the C# extension. That extension will install
.NET Install Tool
Extension.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] please change tabs to spaces
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.
@RussKie I had the same discussion earlier with @danmoseley here. And we agreed on doing it the same as the runtime team is doing. Is there any new information that was discussed internally?
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.
The rest of the file uses spaces, and this part uses tabs. It just looks inconsistent. That's all.
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.
@paule96 This would still be very useful to merge
cc @mitchdenny