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

refactor and mypy up ibm datasource #5509

Merged
merged 7 commits into from
Aug 6, 2024

Conversation

a-dubs
Copy link
Collaborator

@a-dubs a-dubs commented Jul 12, 2024

Proposed Commit Message

refactor: refactor and fix check_untyped_defs in DataSourceIBMCloud.py
    
GH-5445

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Nice updates!

I think we can simplify the metadata transformation a bit more. Would something like this work?

diff --git a/cloudinit/sources/DataSourceIBMCloud.py b/cloudinit/sources/DataSourceIBMCloud.py
index c86d68516..05ca8f056 100644
--- a/cloudinit/sources/DataSourceIBMCloud.py
+++ b/cloudinit/sources/DataSourceIBMCloud.py
@@ -341,7 +341,7 @@ def metadata_from_dir(source_dir: str) -> Dict[str, Any]:
     keys.
 
     This function has a lot in common with ConfigDriveReader.read_v2 but
-    there are a number of inconsistencies, such as key renames and only 
+    there are a number of inconsistencies, such as key renames and only
     presenting a 'latest' version, which make it an unlikely candidate to share
     code.
 
@@ -356,11 +356,11 @@ def metadata_from_dir(source_dir: str) -> Dict[str, Any]:
         return json.loads(blob.decode("utf-8"))
 
     def load_file(
-        path: str, translator: Optional[Callable[[bytes], Any]] = None
+        path: str, translator: Callable[[bytes], Any]
     ) -> Any:
         try:
             raw = util.load_binary_file(path)
-            return translator(raw) if translator else raw
+            return translator(raw)
         except IOError as e:
             LOG.debug("Failed reading path '%s': %s", path, e)
             return None
@@ -370,32 +370,27 @@ def metadata_from_dir(source_dir: str) -> Dict[str, Any]:
     files = [
         # tuples of (results_name, path, translator)
         ("metadata_raw", opath("meta_data.json"), load_json_bytes),
-        ("userdata", opath("user_data"), None),
+        ("userdata", opath("user_data"), lambda x: x),
         ("vendordata", opath("vendor_data.json"), load_json_bytes),
         ("networkdata", opath("network_data.json"), load_json_bytes),
     ]
 
-    raw_results: Dict[str, Optional[bytes]] = {}
-    translated_results: Dict[str, Any] = {}
+    results: Dict[str, Any] = {}
 
     for name, path, transl in files:
         fpath = os.path.join(source_dir, path)
-        raw_results[name] = load_file(fpath)
-        if raw_results[name] is not None and transl is not None:
-            translated_results[name] = load_file(fpath, transl)
-        else:
-            translated_results[name] = raw_results[name]
+        results[name] = load_file(fpath, transl)
 
-    if raw_results["metadata_raw"] is None:
+    if results["metadata_raw"] is None:
         raise sources.BrokenMetadata(
             "%s missing required file 'meta_data.json'",
             source_dir,
         )
 
-    translated_results["metadata"] = {}
+    results["metadata"] = {}
 
-    md_raw = translated_results["metadata_raw"]
-    md = translated_results["metadata"]
+    md_raw = results["metadata_raw"]
+    md = results["metadata"]
 
     if "random_seed" in md_raw:
         try:
@@ -416,7 +411,7 @@ def metadata_from_dir(source_dir: str) -> Dict[str, Any]:
         if old_key in md_raw:
             md[new_key] = md_raw[old_key]
 
-    return translated_results
+    return results
 
 
 # Used to match classes to dependencies

cloudinit/sources/DataSourceIBMCloud.py Show resolved Hide resolved
cloudinit/sources/DataSourceIBMCloud.py Outdated Show resolved Hide resolved
@a-dubs
Copy link
Collaborator Author

a-dubs commented Jul 16, 2024

@TheRealFalcon Thank you for the patch! I really appreciate that since i was definitely still not nearly satisfied with how messy that was even after the refactor.

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

I have a few more minor comments inline.

cloudinit/sources/DataSourceIBMCloud.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceIBMCloud.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceIBMCloud.py Outdated Show resolved Hide resolved
@TheRealFalcon TheRealFalcon self-assigned this Jul 29, 2024
@TheRealFalcon
Copy link
Member

@a-dubs , the changes here look good to me, but you still have some linter failures to fix first

@@ -272,6 +272,8 @@ def test_template_live(self, m_platform, m_sysuuid):
)

ret = ibm.read_md()
if ret is None: # this is needed for mypy - ensures ret is not None
self.fail("read_md returned None unexpectedly")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TheRealFalcon is there a better way I should be doing this? I think this is pretty okay but just curious. thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I'm tempted to disable mypy for tests, but for mypy complaining this looks good!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well now that you mention it, mypy is supposed to be disabled for tests. All I did in this PR was remove the line disabling mypy checks for the ibm datasource file, not the unit test file for it.

@TheRealFalcon
Copy link
Member

LGTM except there's a lint check failing.

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks!

@TheRealFalcon TheRealFalcon merged commit d396de1 into canonical:main Aug 6, 2024
23 checks passed
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.

2 participants