Skip to content
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

Add SQL Server container #82

Closed
wants to merge 6 commits into from
Closed

Add SQL Server container #82

wants to merge 6 commits into from

Conversation

annabaas
Copy link
Contributor

Please let me know if it requires anything else!

@@ -100,3 +101,13 @@ def connect():
cursor = db.restaurants.find({"borough": "Manhattan"})
for document in cursor:
print(document)

@pytest.mark.skip(reason="needs ms sql client libraries unavailable on Travis")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How we can test it then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure! I ran it locally and followed the example for Oracle here... I thought it wouldn't work in Travis because of discussions like this one, but it might just work using sqlalchemy.

@codecov-io
Copy link

codecov-io commented Apr 17, 2020

Codecov Report

Merging #82 into master will decrease coverage by 2.93%.
The diff coverage is 36.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   84.94%   82.01%   -2.94%     
==========================================
  Files          19       20       +1     
  Lines         392      417      +25     
  Branches       25       28       +3     
==========================================
+ Hits          333      342       +9     
- Misses         43       59      +16     
  Partials       16       16              
Impacted Files Coverage Δ
testcontainers/mssql.py 36.00% <36.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e4fa30...de20bcd. Read the comment docs.

@tillahoffmann
Copy link
Collaborator

tillahoffmann commented Apr 17, 2020

Hi @annabaas, thank you for the PR. Could you run make requirements from the root folder of the repository and push the changes (you'll need to have docker installed)? That command will make sure the extra dependencies are picked up.

@annabaas
Copy link
Contributor Author

@tillahoffmann thank you! that should fix the tests (I hope). The script failed for 3.8 ("ModuleNotFoundError: No module named 'Cython'"), I added the req manually for that one as I don't think the issue is related to my PR.

@annabaas
Copy link
Contributor Author

It definitely is related. Sorry :(

@tillahoffmann
Copy link
Collaborator

tillahoffmann commented Apr 17, 2020

It looks like pymysql is no longer being supported and doesn't work well with python3 (see here for details). It looks like that issue also has some pointers to alternative client libraries.

@annabaas
Copy link
Contributor Author

It seems to be nontrivial to get pymssql to work with 3.8, and pymssql support is being withdrawn :( I'll close this PR and open a new one if we get it to work with ODBC or one of the alternatives.

@annabaas annabaas closed this Apr 17, 2020
@tillahoffmann
Copy link
Collaborator

Sounds like a plan. 🙂 Feel free to keep pushing changes here to see whether everything builds/tests as expected, and we can squash merge afterwards.

@annabaas
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants