Skip to content
This repository was archived by the owner on May 31, 2019. It is now read-only.

(bugfix) updated SQLITE3 prevent EF7 crash on boilerplate ASPNET sites #122

Merged
merged 1 commit into from
Jan 4, 2016

Conversation

AlexChesser
Copy link
Contributor

response to issue #121 fixes an issue where database-enabled boilerplates from Yo generators would crash on attempted access

but how cool is this?! aspnet-docker running a container where I've actually registered and logged in locally.

capture

@dnfclas
Copy link

dnfclas commented Dec 3, 2015

Hi @AlexChesser, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented Dec 3, 2015

@AlexChesser, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@friism
Copy link
Contributor

friism commented Dec 3, 2015

hawt

@ahmetb
Copy link
Contributor

ahmetb commented Dec 6, 2015

@AlexChesser are you planning to undo 38ba05e? I think it is not related to this PR.

@AlexChesser
Copy link
Contributor Author

@ahmetalpbalkan yessir! I've just gotten home from work and am planning on taking care of all the tickets I have both here and on omnisharp.

I'm afraid I didn't realize that commits on top of my PR are added to the PR if it hasn't been merged yet.

I'll revert the last two and leave it at JUST the sqllite fix.

As it turns out I think we've agreed that 38ba05e belongs in OmniSharp while it appears the non-standard sqllite3 is OK over here.

@ahmetb
Copy link
Contributor

ahmetb commented Dec 7, 2015

that'd clear up a lot of things, thanks @AlexChesser :)

@AlexChesser
Copy link
Contributor Author

OK @ahmetalpbalkan - hopefully this is now in a state you feel comfortable merging. Let me know if there's anything else you need before it can be folded in (for example, the team over at omnisharp would prefer comments aren't in the dockerfile)

@natemcmaster
Copy link
Contributor

The updated libsqlite3-dev package LGTM.

Just FYI, in RC2 we support back to SQLite 3.7.9, so a newer sqlite3 package is not necessary (current image uses 3.7.13.) See aspnet/Microsoft.Data.Sqlite#171

@glennc
Copy link
Member

glennc commented Dec 15, 2015

I'm ok with this one. @ahmetalpbalkan and @natemcmaster are you both good for this to be merged?

@natemcmaster
Copy link
Contributor

:shipit:

1 similar comment
@ahmetb
Copy link
Contributor

ahmetb commented Dec 15, 2015

:shipit:

@muratg
Copy link

muratg commented Jan 4, 2016

@ahmetalpbalkan @glennc Could one of you guys take this in?

ahmetb added a commit that referenced this pull request Jan 4, 2016
(bugfix) updated SQLITE3 prevent EF7 crash on boilerplate ASPNET sites
@ahmetb ahmetb merged commit 888ef49 into aspnet:master Jan 4, 2016
@shanselman
Copy link

What's the status here? @friism @AlexChesser @ahmetalpbalkan

@AlexChesser
Copy link
Contributor Author

This one looks right - I've been trying to get the performance improvement merged, but have been struggling to find the right place.

Omnisharp leaned towards declining because they want to use the exact dockerfile from aspnet home
OmniSharp/generator-aspnet#531

So I tried to send the PR to home
dotnet/aspnetcore#1313
but the issue is sending me down two other threads dotnet/aspnetcore#1312

I think I've got to find out which repo is the official source for the docker file so I can send the performance improvement to the correct place.

@friism
Copy link
Contributor

friism commented Mar 2, 2016

@shanselman I think someone should think about the suite of different Dockerfiles that'll be used by various .NET developers. It's great that upgrading sqlite makes some specific default samples work out of the box, but the fact that something like sqlite is in the image in the first place is a bit dubious. It means that everyone using this image will be saddled with the sqlite dependency whether they need it or not (eg. they could be building an ASP.NET app that doesn't use Entityframework, and even if it uses Entityframework it'll probably use postgres or sql server (at least in production), and sqlite is just bloating up the image in that case.

Off the top of my head (and I haven't thought this through) one could envision:

  • The dotnet Docker images are absolutely minimal, just barebones .NET runtime, and at ONBUILD they do restore.
  • aspnet-docker images (this repo) that build on the dotnet ones, but add kestrel/libuv and other absolutely required dependencies (not sqlite in my opinion)
  • the yo generators can either maintain their own bigger base images (based on the aspnet one) or they can generate Dockerfiles based on aspnet that optionally adds sqlite based on user choice.

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