Nixops aws compat #1355
Nixops aws compat #1355
Conversation
adisbladis
force-pushed
the
nixops-aws-compat
branch
2 times, most recently
from
5af6d13
to
89fb5f9
Compare
adisbladis
force-pushed
the
nixops-aws-compat
branch
2 times, most recently
from
01015d9
to
3890c44
Compare
nixops/deployment.py
Outdated
| def get_typed_resource( | ||
| self, name: str, type: str | ||
| ) -> nixops.resources.GenericResourceState: | ||
| self, name: str, type_name: str, type: Type[TypedResource] | ||
| ) -> TypedResource: | ||
| res = self.active_resources.get(name, None) | ||
| if not res: | ||
| raise Exception("resource ‘{0}’ does not exist".format(name)) | ||
| if res.get_type() != type: | ||
| raise Exception("resource ‘{0}’ is not of type ‘{1}’".format(name, type)) | ||
| return res | ||
| if res.get_type() != type_name: | ||
| raise Exception( | ||
| "resource ‘{0}’ is not of type ‘{1}’".format(name, type_name) | ||
| ) | ||
| return res # type:ignore | ||
|
|
||
| def get_machine(self, name: str) -> nixops.resources.GenericResourceState: | ||
| def get_machine(self, name: str, type: Type[TypedResource]) -> TypedResource: | ||
| res = self.active_resources.get(name, None) |
There was a problem hiding this comment.
I wonder if we should have a get_generic_machine/resource, and a get_typed_machine/resource. What do you think?
There was a problem hiding this comment.
I was considering returning to
def get_machine(self, name: str) -> nixops.resources.GenericResourceState:and let the caller use cast instead.
This results in exactly the same type safety as
def get_machine(self, name: str, type: Type[TypedResource]) -> TypedResource:with only some slight inconvenience for the caller.
Wdyt?
There was a problem hiding this comment.
I like the second option much better. It is really awkward to use in other libraries: https://github.com/input-output-hk/nixops-packet/pull/21/files#diff-78f3655b013910dfdb430d53dae85085R11-R18 this feels like something that should definitely be implemented in core, to provide a safe API.
nixops/diff.py
Outdated
| @@ -188,7 +188,7 @@ def retrieve_def(d): | |||
| name = d[4:].split(".")[0] | |||
| res_type = d.split(".")[1] | |||
| k = d.split(".")[2] if len(d.split(".")) > 2 else key | |||
| res = self._depl.get_typed_resource(name, res_type) | |||
| res = self._depl.get_typed_resource(name, res_type, Any) | |||
There was a problem hiding this comment.
the _generic method would solve this one a bit nicer I think.
| # While this looks like a rookie mistake where the list is going get shared | ||
| # across all class instances it's not... It's working around a Mypy crash. | ||
| # | ||
| # We're overriding this value in __init__. | ||
| # It's safe despite there being a shared list on the class level |
There was a problem hiding this comment.
Can you include a link to a bug ref, and/or a version number, or some more specific information for future spelunkers to explain exactly what code is related to this, and what they can delete once it is fixed?
| # Support Sequence[ImmutableValidatedObject] | ||
| if isinstance(value, tuple) and not isinstance(ann, str): | ||
| new_value = [] | ||
| for v in value: | ||
| for subann in ann.__args__: | ||
| if inspect.isclass(subann) and issubclass( | ||
| subann, ImmutableValidatedObject | ||
| ): | ||
| new_value.append(subann(**v)) | ||
| else: | ||
| new_value.append(v) | ||
| value = tuple(new_value) | ||
|
|
adisbladis
force-pushed
the
nixops-aws-compat
branch
from
3890c44
to
a2416cb
Compare
By taking a type argument and infering return type from that value
adisbladis
force-pushed
the
nixops-aws-compat
branch
2 times, most recently
from
4f99b53
to
76873ac
Compare
`_wait_for: List["ResourceState"] = []` Results in every instance sharing the same list, causing a resource to wait for itself before destruction.
adisbladis
force-pushed
the
nixops-aws-compat
branch
from
76873ac
to
ffd2712
Compare
Some changes required for NixOS/nixops-aws#103 to land.