Skip to content
This repository was archived by the owner on Nov 16, 2023. It is now read-only.

Add support for .NET 4.7#29

Merged
shirhatti merged 5 commits into
masterfrom
dotnet47
Jun 13, 2017
Merged

Add support for .NET 4.7#29
shirhatti merged 5 commits into
masterfrom
dotnet47

Conversation

@richlander

@richlander richlander commented Jun 5, 2017

Copy link
Copy Markdown
Member

@msftclas

msftclas commented Jun 5, 2017

Copy link
Copy Markdown

@richlander,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@MichaelSimons

Copy link
Copy Markdown
Member

@richlander, were you going to update the README.md to include the new tags as well?

Comment thread 3.5/Dockerfile Outdated
rd /s /q C:\build & \
RUN powershell -Command Add-WindowsFeature Web-Server & \
powershell -Command Add-WindowsFeature Web-Asp-Net & \
%windir%\SysWOW64\inetsrv\appcmd set app "Default Web Site/" /applicationPool:".NET v2.0" & \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of changing the AppPool, we should changed .NET Framework version of the DefaultAppPool to .NET 2.0 to work with the new ServiceMonitor.exe
See microsoft/iis-docker#41

Comment thread 4.6.2/Dockerfile
RUN powershell -Command Add-WindowsFeature NET-Framework-45-ASPNET; \
powershell -Command Add-WindowsFeature Web-Asp-Net45; \
powershell -Command Remove-Item -Recurse C:\inetpub\wwwroot\*
SHELL ["powershell", "-Command", "$ErrorActionPreference = 'Stop'; $ProgressPreference = 'SilentlyContinue';"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This change broke our docker environments as we were expecting a default shell of cmd /s /c for running a .msi file. Easily fixed, but worth noting for future releases.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This surprised me as well; this is a breaking change for an existing tag. If a PowerShell version was desirable for an existing tag, it would've been better to add a new image version and tag (e.g. microsoft/aspnet:4.6.2-powershell)

I'm triaging an issue reported for Docker; moby/moby#34013 and suspect this may be the cause

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for reporting this! I didn't see the earlier comment from @FFLSH. We are going to discuss this today and decide what to do.

Would you like us to change back now or stay as-is at this point?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given that this has already been released, reverting it is likely to cause more headaches for people who have updated their dockerfiles to counter this change, so I'd leave it as-is

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

idk what's best @friism @tianon any suggestions here?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Our experience is that once a breaking change is made, it's definitely more headache to try and revert it, since a non-zero number of your users will have already adapted to the change. There's not really a good way to handle any of this sort of thing. 😞

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree. It's a screwup on my part. I'm really sorry about that. In retrospect, pushing everyone to powershell as the default shell should either be done in the servercore base image or not at all.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Personally I use PowerShell in all the images I build too, so I understand why the ASP.NET team would want to make this change.

It would be nice, however, if all Windows "base" images had mostly similar defaults, and in this case that would be cmd for the shell...

As @tianon mentioned I understand if you don't want to change this back.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps a convention should be used for PowerShell / non-PowerShell images. Similar to many images having a -alpine variant, allowing users to either use alpine as a base, or debian.

If PowerShell will be the default, having a -cmdexe (or whatever) as an alternative would help for that.

A brief description of the differences (e.g. escaping, if different) in the readme would be good for those

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants