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

feat: Add cloud version string #413

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Sep 10, 2024

No description provided.

Copy link
Collaborator

@a-dubs a-dubs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a super quick unit test to each lxd and qemu cloud unit test files?

@@ -112,6 +112,8 @@ def test_public_api(cloud: BaseCloud):
# Not sure there's a great way to test this other than not raising
cloud.image_serial(image_id)

version = cloud.version()
assert isinstance(version, str) or version is None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert isinstance(version, str) or version is None
assert isinstance(version, str) or version is None

@@ -260,6 +260,10 @@ def use_key(self, public_key_path, private_key_path=None, name=None):
self._log.debug("using SSH key from %s", public_key_path)
self.key_pair = KeyPair(public_key_path, private_key_path, name)

def version(self) -> Optional[str]:
"""Version string of the platform."""
return None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a lot of value in us returning None here instead of just an empty string ""? We can then avoid the Optional[str] handling for typing validation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with chad. I don't think there is much benefit to returning a None instead of a falsey/empty string.

@@ -660,3 +660,12 @@ def clean(self) -> List[Exception]:
exceptions.append(e)

return exceptions

def version(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holmanb We should probably lru_cache this as it's unlikely to change mid integration test run, but it's something we don't want to recalculate any time we call it.

@@ -437,6 +437,10 @@ def clean(self) -> List[Exception]:
exceptions.append(e)
return exceptions

def version(self):
Copy link
Collaborator

@blackboxsw blackboxsw Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, I'd cache this value somehow as it's unlikely to change on the host mid-test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants