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

Go back to python's sqlite module, not pysqlite2 #1119

Closed
wants to merge 3 commits into from

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Mar 30, 2019

Commit history suggests this was only meant as a temporary
workaround, and judging by history of each pysqlite2 is done
(presumably because it lives in python2 tree now)

https://pypi.org/project/pysqlite/#history
(Aug 2016)

vs.

(Actively developed)
https://github.com/python/cpython/commits/2.7/Modules/_sqlite

Transactional again, this was meant to be temporary.
Comment indicates python 2.7.13 has the fix,
and this seems available back a few releases:

18.09: 2.7.15
18.03: 2.7.15
17.09: 2.7.14
17.03: 2.7.13
16.09: 2.7.13
--------------
16.03: 2.7.12

This reverts commit e86e5f2.
@AmineChikhaoui
Copy link
Member

Seems to break things for some reason, e.g one of the VPC tests:

======================================================================
ERROR: tests.functional.vpc.TestVPC.test_deploy_nat_gtw
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/nix/store/df6jxnkkn3diyrvrzpqx1gs99j0s9m9x-python2.7-nose-1.3.7/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/amine/src/nixops/tests/functional/vpc.py", line 62, in test_deploy_nat_gtw
    self.depl.deploy(plan_only=True)
  File "/home/amine/src/nixops/nixops/deployment.py", line 1063, in deploy
    self.run_with_notify('deploy', lambda: self._deploy(**kwargs))
  File "/home/amine/src/nixops/nixops/deployment.py", line 1052, in run_with_notify
    f()
  File "/home/amine/src/nixops/nixops/deployment.py", line 1063, in <lambda>
    self.run_with_notify('deploy', lambda: self._deploy(**kwargs))
  File "/home/amine/src/nixops/nixops/deployment.py", line 917, in _deploy
    r.index = self._get_free_resource_index()
  File "/home/amine/src/nixops/nixops/util.py", line 258, in set
    else: self._set_attr(name, x)
  File "/home/amine/src/nixops/nixops/resources/__init__.py", line 80, in _set_attr
    self._set_attrs({name: value})
  File "/home/amine/src/nixops/nixops/resources/__init__.py", line 76, in _set_attrs
    (self.id, n, v))
IntegrityError: FOREIGN KEY constraint failed

@AmineChikhaoui
Copy link
Member

since it's not urgent, I might get back to it at some point to investigate further.

@dtzWill
Copy link
Member Author

dtzWill commented Apr 23, 2019 via email

@AmineChikhaoui
Copy link
Member

@dtzWill This is not during the build, this is while running functional tests that do spin up some machines/resources and test things get deployed/destroyed etc ..

@grahamc
Copy link
Member

grahamc commented Mar 26, 2020

Hello!

Thank you for this PR.

In the past several months, some major changes have taken place in
NixOps:

  1. Backends have been removed, preferring a plugin-based architecture.
    Here are some of them:

  2. NixOps Core has been updated to be Python 3 only, and at the
    same time, MyPy type hints have been added and are now strictly
    required during CI.

This is all accumulating in to what I hope will be a NixOps 2.0
release
. There is a tracking issue for that:
#1242 . It is possible that
more core changes will be made to NixOps for this release, with a
focus on simplifying NixOps core and making it easier to use and work
on.

My hope is that by adding types and more thorough automated testing,
it will be easier for contributors to make improvements, and for
contributions like this one to merge in the future.

However, because of the major changes, it has become likely that this
PR cannot merge right now as it is. The backlog of now-unmergable PRs
makes it hard to see which ones are being kept up to date.

If you would like to see this merge, please bring it up to date with
master and reopen it
. If the or mypy type checking fails, please
correct any issues and then reopen it. I will be looking primarily at
open PRs whose tests are all green.

Thank you again for the work you've done here, I am sorry to be
closing it now.

Graham

@grahamc grahamc closed this Mar 26, 2020
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.

None yet

3 participants