Browse Source

Fix integration regressions from the typing work

Two behavioral regressions surfaced by the per-cloud integration tests:

- GCP VM firewall creation broke. `vm_firewalls.create` adds the implicit
  default-egress rule via `create_with_priority`, but that rule matches
  `is_dummy_rule()` and is deliberately hidden by `list()`/`find()`, so the
  find-after-create returns nothing. The typed refactor had turned that
  "not found" case from `return None` into a raise, so firewall creation
  (and everything depending on it) failed. Restore `create_with_priority`
  returning `None` for the unfindable dummy rule (its callers ignore the
  return); the public, interface-facing `create()` still raises for a
  genuinely missing rule.

- `test_instance_start_stop_methods` asserted a truthy return from
  `stop()`/`start()`, but those now return `None` per the interface. The
  test already verifies the real effect via `wait_for` + state assertions,
  so the obsolete return-value assertions are removed.

mypy 2.1 and flake8 clean. GCP/AWS integration paths are validated by CI
(not reproducible locally without cloud credentials).
Nuwan Goonasekera 1 month ago
parent
commit
6ef256d760
2 changed files with 14 additions and 13 deletions
  1. 12 5
      cloudbridge/providers/gcp/services.py
  2. 2 8
      tests/test_compute_service.py

+ 12 - 5
cloudbridge/providers/gcp/services.py

@@ -329,7 +329,8 @@ class GCPVMFirewallRuleService(BaseVMFirewallRuleService):
             protocol: str | None, priority: int, from_port: int | None = None,
             to_port: int | None = None,
             cidr: str | builtins.list[str] | None = None,
-            src_dest_fw: VMFirewall | None = None) -> GCPVMFirewallRule:
+            src_dest_fw: VMFirewall | None = None
+            ) -> GCPVMFirewallRule | None:
         gcp_fw = cast(GCPVMFirewall, firewall)
         port = GCPVMFirewallRuleService.to_port_range(from_port, to_port)
         src_dest_tag = None
@@ -347,11 +348,13 @@ class GCPVMFirewallRuleService(BaseVMFirewallRuleService):
                           from_port=from_port, to_port=to_port, cidr=cidr,
                           src_dest_fw_id=src_dest_fw_id)
         if len(rules) < 1:
-            raise ProviderInternalException(
-                "VM firewall rule not found after creation")
+            # The implicit default-egress rule that VMFirewall creation adds is
+            # hidden by list()/find() as a "dummy" rule, so it is not findable
+            # here. Callers that add it ignore the return value; the public
+            # create() below raises for a genuinely missing rule.
+            return None
         return cast(GCPVMFirewallRule, rules[0])
 
-    # declares a non-optional VMFirewallRule.
     @dispatch(event="provider.security.vm_firewall_rules.create",
               priority=BaseVMFirewallRuleService.STANDARD_EVENT_PRIORITY)
     def create(self, firewall: VMFirewall,
@@ -361,9 +364,13 @@ class GCPVMFirewallRuleService(BaseVMFirewallRuleService):
                cidr: str | builtins.list[str] | None = None,
                src_dest_fw: VMFirewall | None = None
                ) -> GCPVMFirewallRule:
-        return self.create_with_priority(firewall, direction, protocol,
+        rule = self.create_with_priority(firewall, direction, protocol,
                                          1000, from_port, to_port, cidr,
                                          src_dest_fw)
+        if rule is None:
+            raise ProviderInternalException(
+                "VM firewall rule not found after creation")
+        return rule
 
     # The interface declares delete(firewall, rule_id: str); this impl also
     # accepts a GCPVMFirewallRule instance.

+ 2 - 8
tests/test_compute_service.py

@@ -436,7 +436,7 @@ class CloudComputeServiceTestCase(ProviderTestBase):
                                                   subnet=subnet)
 
             # check whether stopping aws instance works
-            resp = test_inst.stop()
+            test_inst.stop()
             test_inst.wait_for([InstanceState.STOPPED])
             test_inst.refresh()
             self.assertTrue(
@@ -445,11 +445,8 @@ class CloudComputeServiceTestCase(ProviderTestBase):
                 "'stop' operation but got %s"
                 % test_inst.state)
 
-            self.assertTrue(resp, "Response from method was suppose to be"
-                            + " True but got False")
-
             # check whether starting aws instance works
-            resp = test_inst.start()
+            test_inst.start()
             test_inst.wait_for([InstanceState.RUNNING])
             test_inst.refresh()
             self.assertTrue(
@@ -457,6 +454,3 @@ class CloudComputeServiceTestCase(ProviderTestBase):
                 "Instance state must be running when refreshing after a "
                 "'start' operation but got %s"
                 % test_inst.state)
-
-            self.assertTrue(resp, "Response from method was suppose to be"
-                            + " True but got False")