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

Plugin types: Tie a ResourceState to its corresponding ResourceDefinition #1346

Merged
merged 22 commits into from May 18, 2020

Conversation

@grahamc
Copy link
Member

grahamc commented May 16, 2020

Using a protocol here makes mypy much more picky about variable definitions in the base classes (I think this is a good thing.) This helped find some hidden assumptions which are more correctly typed now.

grahamc added 19 commits May 16, 2020
…ction
…ed when fetching defns for machine-only actions
… _wait_for Optional
@grahamc grahamc force-pushed the grahamc:plugin-types branch 2 times, most recently from c32739d to ece26ef May 16, 2020
Otherwise, all the references have an Any
@grahamc grahamc force-pushed the grahamc:plugin-types branch from ece26ef to 83c700a May 16, 2020
@grahamc
Copy link
Member Author

grahamc commented May 16, 2020

I'm not sure about this last commit, but I think we do need to specify it is as generic as possible.

@grahamc grahamc requested a review from adisbladis May 16, 2020
adisbladis and others added 2 commits May 18, 2020
We'll get bogus hits, only check the concrete type.
Explicitly subclassed Protocols don't support super().__init__
... but we need that.

See: python/typing#572 for a description, including
the below workaround.

Protocol doesn't give us any special run-time behavior (except for
runtime_checkable,) and can be pretty transparently swapped out for
Generic at run time.

By using Generic at run-time, we get the expected __init__ behavior.

But, we still want Protocols at type-checking time because Protocol
is much stricter about assigning to `self` without explicitly defining
and typing the object variable.

In conclusion, I'm sorry. Hopefully
python/typing#572 gets fixed and we can
delete this and go back to the isinstance check in deployment.py.

Co-authored-by: Graham Christensen <graham.christensen@tweag.io>
@adisbladis adisbladis force-pushed the grahamc:plugin-types branch from 163dc3d to b6abe7b May 18, 2020
@grahamc grahamc merged commit 7f38a16 into NixOS:master May 18, 2020
10 checks passed
10 checks passed
parsing
Details
build
Details
black
Details
mypy
Details
flake8
Details
mypy-ratchet
Details
coverage
Details
docs
Details
poetry-up-to-date
Details
docs/readthedocs.org:nixops Read the Docs build succeeded!
Details
@grahamc grahamc deleted the grahamc:plugin-types branch May 18, 2020
@@ -1488,7 +1489,8 @@ def worker(m: nixops.resources.ResourceState) -> None:
m._errored = True
raise
finally:
m._destroyed_event.set()
if dep._destroyed_event is not None:

This comment has been minimized.

Copy link
@tewfik-ghariani

tewfik-ghariani May 18, 2020

Contributor

cc @grahamc this triggers the following error

    if dep._destroyed_event is not None:
UnboundLocalError: local variable 'dep' referenced before assignment

This comment has been minimized.

Copy link
@grahamc

grahamc May 18, 2020

Author Member

omg!

This comment has been minimized.

Copy link
@grahamc

grahamc May 18, 2020

Author Member

How did you catch this error? and why didn't I? :/

This comment has been minimized.

Copy link
@tewfik-ghariani

tewfik-ghariani May 18, 2020

Contributor

Randomly xD

Was playing around a deployment containing some missing resource

+-----------------------+---------+-------------------+---------------------------------------+------------+
| Name                  |  Status | Type              | Resource Id                           | IP address |
+-----------------------+---------+-------------------+---------------------------------------+------------+
| test                  |    Up   | command-output    | test-afc21edad3e1050cde1bde655014dd59 |            |
| firstGrafanaDashboard | Missing | grafana-dashboard |                                       |            |
+-----------------------+---------+-------------------+---------------------------------------+------------+

And when I tried to destroy all resources, came across that error 🤞

$ nixops destroy -d grafana-test  
warning: are you sure you want to destroy test? (y/N) y
test> destroying...
Traceback (most recent call last):
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/bin/nixops", line 11, in <module>
    load_entry_point('nixops', 'console_scripts', 'nixops')()
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/__main__.py", line 705, in main
    args.op(args)
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/script_defs.py", line 632, in op_destroy
    include=args.include or [], exclude=args.exclude or [], wipe=args.wipe
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/deployment.py", line 1506, in destroy_resources
    "destroy", lambda: self._destroy_resources(include, exclude, wipe)
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/deployment.py", line 1366, in run_with_notify
    f()
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/deployment.py", line 1506, in <lambda>
    "destroy", lambda: self._destroy_resources(include, exclude, wipe)
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/deployment.py", line 1496, in _destroy_resources
    nr_workers=-1, tasks=list(self.resources.values()), worker_fun=worker
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/parallel.py", line 106, in run_tasks
    raise list(exceptions.values())[0]
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/parallel.py", line 70, in thread_fun
    work_result = (worker_fun(t), None, t.name)
  File "/home/tewfikghariani/.cache/pypoetry/virtualenvs/nixops-grafana-FCCl2_6c-py3.7/src/nixops/nixops/deployment.py", line 1492, in worker
    if dep._destroyed_event is not None:
UnboundLocalError: local variable 'dep' referenced before assignment

This comment has been minimized.

Copy link
@grahamc

grahamc May 18, 2020

Author Member

Thanks, @tewfic-ghariani! Oops! I've opened up a fixup PR. Can you give it a test? #1348

This comment has been minimized.

Copy link
@tewfik-ghariani

tewfik-ghariani May 18, 2020

Contributor

Works fine now! Thanks

dep._destroyed_event.wait()
if dep._created_event is not None:
dep._created_event.wait()
Comment on lines -1478 to +1479

This comment has been minimized.

Copy link
@grahamc

grahamc May 18, 2020

Author Member

This was buggy, too.

grahamc added a commit to grahamc/nixops that referenced this pull request May 18, 2020
adisbladis added a commit that referenced this pull request May 19, 2020
deployment.py: fixup mistakes in #1346
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

3 participants
You can’t perform that action at this time.