Skip to content

Conversation

jelster
Copy link
Contributor

@jelster jelster commented Jan 7, 2017

No description provided.

@jelster
Copy link
Contributor Author

jelster commented Jan 7, 2017

sorry for the spurious change in one of those files -- not sure why what appeared to be a nop change turned into a merge conflict (need to check my EOL and whitespace settings)

Copy link
Contributor

@uc-msft uc-msft left a comment

Choose a reason for hiding this comment

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

Please use single quotes around literals - parameters to sp_detach_db for example. Script has bug in that if you have database name like "my db" it will fail. Similarly, use quoted identifier around the database name in the CREATE DATABASE statement.

@jelster
Copy link
Contributor Author

jelster commented Jan 27, 2017

Hi @uc-msft - thanks for the feedback. Can you elaborate re?

 Script has bug in that if you have database name like "my db" it will fail

Using single quotes is something I initially looked into doing, however the problem with that is that in both *NIX and Powershell, single quotes are enforced as literals, and thus environment variables are not expanded.

I found it much easier for the purposes of minimizing change to the sample to instead continue the pre-existing need for users to pass in database names surrounded by quoted identifiers, e.g: "database: { name: "'my db'", "files": [...

How much logic would you think is most appropriate to add for parameter checking?

@perrysk-msft
Copy link
Contributor

Thanks @jelster!

I am closing this PR as these files have been moved to avoid duplication: https://github.com/Microsoft/mssql-docker/tree/master/windows

Also the 2014 express image has been deprecated.

@jelster jelster deleted the containers-startPS branch February 8, 2017 22:58
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.

3 participants