Skip to content

Commit

Permalink
chore: update according to code reviews
Browse files Browse the repository at this point in the history
  • Loading branch information
IronCore864 committed Mar 20, 2024
1 parent 0179902 commit c160605
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 26 deletions.
38 changes: 18 additions & 20 deletions ops/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -3625,30 +3625,28 @@ def _ensure_loaded(self):
class CloudCredential:
"""Credentials for cloud.
Note that the credential might contain redacted secrets in the `redacted` field.
Used as the type of attribute `credential` in :class:`CloudSpec`.
"""

auth_type: str
"""Authentication type."""

attributes: Dict[str, str]
attributes: Optional[Dict[str, str]] = dataclasses.field(default_factory=dict)

This comment has been minimized.

Copy link
@benhoyt

benhoyt Mar 20, 2024

Collaborator

The fields with defaults other than None shouldn't be typed as Optional, because they'll never be None, and we don't want the type annotation to say "you have to check that spec.attributes is not None before you access it. So let's remove the Optional that you added to the type annotation from attributes, redacted, ca_certificates, skip_tls_verify, and is_controller_cloud.

Other than that, this looks good now!

"""A dictionary containing cloud credentials.
For example, for AWS, it contains `access-key` and `secret-key`;
for Azure, `application-id`, `application-password` and `subscription-id`
can be found here.
"""

redacted: List[str]
redacted: Optional[List[str]] = dataclasses.field(default_factory=list)
"""A list of redacted secrets."""

@classmethod
def from_dict(cls, d: Dict[str, Any]) -> 'CloudCredential':
"""Create a new CloudCredential object from a dictionary."""
return cls(
auth_type=d.get('auth-type') or '',
auth_type=d['auth-type'],
attributes=d.get('attrs') or {},
redacted=d.get('redacted') or [],
)
Expand All @@ -3664,28 +3662,28 @@ class CloudSpec:
name: str
"""Juju cloud name."""

region: Optional[str]
region: Optional[str] = None
"""Region of the cloud."""

endpoint: Optional[str]
endpoint: Optional[str] = None
"""Endpoint of the cloud."""

identity_endpoint: Optional[str]
identity_endpoint: Optional[str] = None
"""Identity endpoint of the cloud."""

storage_endpoint: Optional[str]
storage_endpoint: Optional[str] = None
"""Storage endpoint of the cloud."""

credential: Optional[CloudCredential]
"""Cloud credentials, an object of type :class:`CloudCredential`."""
credential: Optional[CloudCredential] = None
"""Cloud credentials with key-value attributes."""

ca_certificates: List[str]
ca_certificates: Optional[List[str]] = dataclasses.field(default_factory=list)
"""A list of CA certificates."""

skip_tls_verify: bool
"""Whether to skip TLS verfication, boolean, defaults to False."""
skip_tls_verify: Optional[bool] = False
"""Whether to skip TLS verfication."""

is_controller_cloud: bool
is_controller_cloud: Optional[bool] = False
"""If this is the cloud used by the controller, defaults to False."""

@classmethod
Expand All @@ -3694,11 +3692,11 @@ def from_dict(cls, d: Dict[str, Any]) -> 'CloudSpec':
return cls(
type=d['type'],
name=d['name'],
region=d.get('region') or '',
endpoint=d.get('endpoint') or '',
identity_endpoint=d.get('identity-endpoint') or '',
storage_endpoint=d.get('storage-endpoint') or '',
credential=CloudCredential.from_dict(d.get('credential') or {}),
region=d.get('region') or None,
endpoint=d.get('endpoint') or None,
identity_endpoint=d.get('identity-endpoint') or None,
storage_endpoint=d.get('storage-endpoint') or None,
credential=CloudCredential.from_dict(d['credential']) if d.get('credential') else None,
ca_certificates=d.get('cacertificates') or [],
skip_tls_verify=d.get('skip-tls-verify') or False,
is_controller_cloud=d.get('is-controller-cloud') or False,
Expand Down
36 changes: 31 additions & 5 deletions test/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -3798,6 +3798,8 @@ def test_from_dict(self):
}
cloud_cred = ops.CloudCredential.from_dict(d)
self.assertEqual(cloud_cred.auth_type, d['auth-type'])
self.assertEqual(cloud_cred.attributes, {})
self.assertEqual(cloud_cred.redacted, [])

def test_from_dict_full(self):
d = {
Expand Down Expand Up @@ -3825,11 +3827,11 @@ def test_from_dict(self):
)
self.assertEqual(cloud_spec.type, 'lxd')
self.assertEqual(cloud_spec.name, 'localhost')
self.assertEqual(cloud_spec.region, '')
self.assertEqual(cloud_spec.endpoint, '')
self.assertEqual(cloud_spec.identity_endpoint, '')
self.assertEqual(cloud_spec.storage_endpoint, '')
self.assertEqual(cloud_spec.credential, ops.CloudCredential.from_dict({}))
self.assertEqual(cloud_spec.region, None)
self.assertEqual(cloud_spec.endpoint, None)
self.assertEqual(cloud_spec.identity_endpoint, None)
self.assertEqual(cloud_spec.storage_endpoint, None)
self.assertIsNone(cloud_spec.credential)
self.assertEqual(cloud_spec.ca_certificates, [])
self.assertEqual(cloud_spec.skip_tls_verify, False)
self.assertEqual(cloud_spec.is_controller_cloud, False)
Expand Down Expand Up @@ -3868,6 +3870,30 @@ def test_from_dict_full(self):
self.assertEqual(cloud_spec.skip_tls_verify, False)
self.assertEqual(cloud_spec.is_controller_cloud, True)

def test_from_dict_no_credential(self):
d = {
'type': 'lxd',
'name': 'localhost',
'region': 'localhost',
'endpoint': 'https://10.76.251.1:8443',
'identity-endpoint': 'foo',
'storage-endpoint': 'bar',
'cacertificates': ['foo', 'bar'],
'skip-tls-verify': False,
'is-controller-cloud': True,
}
cloud_spec = ops.CloudSpec.from_dict(d)
self.assertEqual(cloud_spec.type, d['type'])
self.assertEqual(cloud_spec.name, d['name'])
self.assertEqual(cloud_spec.region, d['region'])
self.assertEqual(cloud_spec.endpoint, d['endpoint'])
self.assertIsNone(cloud_spec.credential)
self.assertEqual(cloud_spec.identity_endpoint, d['identity-endpoint'])
self.assertEqual(cloud_spec.storage_endpoint, d['storage-endpoint'])
self.assertEqual(cloud_spec.ca_certificates, d['cacertificates'])
self.assertEqual(cloud_spec.skip_tls_verify, False)
self.assertEqual(cloud_spec.is_controller_cloud, True)


class TestGetCloudSpec(unittest.TestCase):
def setUp(self):
Expand Down
2 changes: 1 addition & 1 deletion test/test_testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -5873,7 +5873,7 @@ def _on_start(self, event: ops.StartEvent):
'type': 'lxd',
'endpoint': 'https://127.0.0.1:8443',
'credential': {
'authtype': 'certificate',
'auth-type': 'certificate',
'attrs': {
'client-cert': 'foo',
'client-key': 'bar',
Expand Down

0 comments on commit c160605

Please sign in to comment.