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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Key value state + tests refactoring #969

Closed
wants to merge 129 commits into from
Closed

Key value state + tests refactoring #969

wants to merge 129 commits into from

Conversation

@srghma
Copy link
Contributor

@srghma srghma commented Jun 14, 2018

This pr consumes:

Tests done:

TODOs:

  • each test has its own store file (for tests parallelization)
  • python2 tests.py tests.functional.test_cloning_clones -vv once in 5 times throws error https://pastebin.com/fe7njGJH
  • tests.functional.test_stopping_stops with ec2 issue https://pastebin.com/BJwKMWti (fix 898107c). I suppose this bug occured in this test because aws returns termination responce faster if instance already stopped, just after that nixops tries to remove security group, and fails, try remove fix and you will see it youself
  • tests.functional.test_stopping_stops with libvirt issue https://pastebin.com/e4VLG8Cc
  • tests.functional.test_ec2_rds_dbinstance not working #970
  • python2 tests.py tests.functional.test_vpc:test_deploy_vpc -vv no working with json state file https://pastebin.com/N8E90uke (fix e1b9158)
  • python2 tests.py tests.functional.test_vpc:test_deploy_vpc_machine -vv not working (https://pastebin.com/hqYBi7hF)

CONSIDER (dont mind it 馃槃, just thoughts):

  • not use StateDict at all because it makes return value type vague, you can put array but get string, use nixops.utils.attr_property instead (also StateDict should be named ResourceStateDict, because only ResourceState can use it)
  • substitute every isinstance(XXX, basestring) with isinstance(XXX, six.text_type)
  • all nixops.util.attr_property should be wrapped in with self.db, maybe highlight their name like self.state_prop_XXX
  • in tests.functional.shared.destroy_deployments_and_remove_state_file dont ignore 'VirtualBoxState' object has no attribute 'get_backups' error https://pastebin.com/W228z0rN
moretea added 30 commits Mar 13, 2017
Needed to faclitate multiple backends (like sqlite3/other remote database)
For now it's just the underlying DB, but it resembles something that
can roll back the current transaction.
while backups[backup_id]['status'] == "running":
time.sleep(10)
backups = deployment.get_backups()
deployment_run_command(deployment, "rm {}/back-me-up".format(path))

This comment has been minimized.

@coretemp

coretemp Nov 5, 2018

Please run any commands via an API that uses arrays to process commands. E.g. foo(["ls","-l"]) vs foo("ls -l").

This comment has been minimized.

@srghma

srghma Nov 10, 2018
Author Contributor

@coretemp

cons:

  1. more characters to print
  2. harder to use .format method with multiple {}. e.g.
deployment_run_command(deployment, "rm {}/back-me-up {}/and-me".format(path))

vs

deployment_run_command(deployment, ["rm", "{}/back-me-up".format(path), "{}/and-me".format(path)])

This comment has been minimized.

@coretemp

coretemp Nov 12, 2018

I don't agree, but perhaps others do. I.e., if it was my code base, it wouldn't get in, but it's not my play ground.

# If your account doesn't support EC2-Classic, you will get an error:
# `VPC DB Security Groups cannot be modified with this API version.
# Please use an API version between 2012-01-15 and 2012-10-31 to modify this group.`
# TODO: remove it?

This comment has been minimized.

@coretemp

coretemp Nov 5, 2018

Removing it seems reasonable or otherwise it should check for EC2-Classic support.

This comment has been minimized.

@srghma

srghma Nov 10, 2018
Author Contributor

@AmineChikhaoui what do you think, remove this comment and ec2-rds-dbinstance-with-sg.nix file OR leave commented and ec2-rds-dbinstance-with-sg.nix file for legacy?

I'm asking because you are author of this test which I can't run https://github.com/NixOS/nixops/blob/aa3602d03cd03fc748cdcf1fc25d97ef863474f0/tests/functional/ec2-rds-dbinstance-with-sg.nix

This comment has been minimized.

@coretemp

coretemp Nov 12, 2018

Checking for classic support is less work then asking him.

from tests.functional.shared.create_deployment import create_deployment
from tests.functional.shared.using_unique_state_file import using_unique_state_file

parent_dir = path.dirname(__file__)

This comment has been minimized.

@coretemp

coretemp Nov 5, 2018

Same comments as before about __file__

@srghma
Copy link
Contributor Author

@srghma srghma commented Nov 10, 2018

NOTE: how to run test

to show all tests:

python2 tests.py --collect-only -v

to run only one test:

# for test named
# test_device_name_to_boto_expected (tests.unit.test_device_name_to_boto_expected.TestDeviceNameToBotoExpected) ... ok

python2 tests.py tests.unit.test_device_name_to_boto_expected

# for tests generated by @parametrized (there is no way to run only one test of these)

# tests.functional.test_send_keys_sends_keys.test_send_keys_sends_keys('json', ('vbox', ['/home/srghma/projects/nixops/tests/functional/shared/nix_expressions/logical_base.nix', '/home/srghma/projects/nixops/tests/functional/shared/nix_expressions/vbox_base.nix'])) ... ok
# tests.functional.test_send_keys_sends_keys.test_send_keys_sends_keys('json', ('libvirtd', ['/home/srghma/projects/nixops/tests/functional/shared/nix_expressions/logical_base.nix', '/home/srghma/projects/nixops/tests/functional/shared/nix_expressions/libvirtd_base.nix'])) ... ok
# tests.functional.test_send_keys_sends_keys.test_send_keys_sends_keys('json', ('ec2', ['/home/srghma/projects/nixops/tests/functional/shared/nix_expressions/logical_base.nix', '/home/srghma/projects/nixops/tests/functional/shared/nix_expressions/ec2_base.nix'])) ... ok
# .....

python2 tests.py tests.functional.test_send_keys_sends_keys.test_send_keys_sends_keys

Proof, that tests, that changed after git merge are working

@coretemp
Copy link

@coretemp coretemp commented Nov 12, 2018

Great that you show a limited form proof that the feature is working. 馃憤

I think I will leave the rest to others (because I don't have merge rights anyway).

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Nov 16, 2018

This quite a massive pull request. I wonder we should split it up into smaller ones so the nixops maintainer do not run away, when having to review this.

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Nov 17, 2018

If your pull request contains
new libraries they can go to this file:

https://github.com/NixOS/nixops/blob/master/setup.cfg

If you want to run the typecheck on your local checkout either run:

$ nix-build --quiet release.nix -A build.x86_64-linux

or

$ nix run nixpkgs.mypy -c mypy nixops
@srghma
Copy link
Contributor Author

@srghma srghma commented Nov 18, 2018

@Mic92 tnx for the suggestion, added mypy-parametrized to setup.cfg

@srghma srghma force-pushed the srghma:kv-state branch from 7d2105c to 16e0b6c Nov 18, 2018
@srghma
Copy link
Contributor Author

@srghma srghma commented Nov 18, 2018

@rbvermaa could you please review this PR

@tbenst
Copy link

@tbenst tbenst commented Dec 4, 2019

@srghma anything I or others can do to help with this PR? Since there鈥檚 interest from core devs Would be awesome to get this merged. I鈥檓 guessing the new plugin architecture means this needs to be (possibly non-trivially) rebased?

@@ -0,0 +1 @@
fix: coretemp review -> fix typo -> change of -

This comment has been minimized.

@srghma

srghma Dec 5, 2019
Author Contributor

remove file

@srghma
Copy link
Contributor Author

@srghma srghma commented Dec 5, 2019

@srghma anything I or others can do to help with this PR? Since there鈥檚 interest from core devs Would be awesome to get this merged. I鈥檓 guessing the new plugin architecture means this needs to be (possibly non-trivially) rebased?

hi, I'm sorry, I won't participate, just have no time

@moretea , since this is basically your PR (I just added test matrix and made sure everything is working) , what do you think about rebasing your commits?

@grahamc
Copy link
Member

@grahamc 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can鈥檛 perform that action at this time.