Plugin types: Tie a ResourceState to its corresponding ResourceDefinition #1346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ed when fetching defns for machine-only actions
… _wait_for Optional
grahamc
force-pushed
the
plugin-types
branch
2 times, most recently
from
c32739d
to
ece26ef
Compare
Otherwise, all the references have an Any
|
I'm not sure about this last commit, but I think we do need to specify it is as generic as possible. |
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>
| @@ -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: | |||
There was a problem hiding this comment.
cc @grahamc this triggers the following error
if dep._destroyed_event is not None:
UnboundLocalError: local variable 'dep' referenced before assignment
There was a problem hiding this comment.
How did you catch this error? and why didn't I? :/
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks, @tewfic-ghariani! Oops! I've opened up a fixup PR. Can you give it a test? #1348
There was a problem hiding this comment.
Works fine now! Thanks
grahamc
commented
May 18, 2020
Comment on lines
-1478
to
+1479
| dep._destroyed_event.wait() | ||
| if dep._created_event is not None: | ||
| dep._created_event.wait() |
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.