Skip to content

Conversation

philogicae
Copy link
Member

Problem: when creating non-coco instances, trusted_execution field is populated
Solution: To not include it

Note: CCN will still put "trusted_execution": null in the final processed message.

@philogicae philogicae requested a review from nesitor August 27, 2024 16:26
Copy link


---

### Summary and Explanation

The provided PR modifies the `create_instance` function in the `authenticated_http.py` file. The changes include:

1. **Refactoring**: The `InstanceEnvironment` dictionary is now created using the `**environment_kwargs` unpacking method. This improves readability and maintainability of the code.

2. **Documentation**: There are no changes to the documentation.

3. **Risk Assessment**: The changes are relatively minor and do not introduce any significant risks. They are focused on improving code readability and organization.

### Highlighting Relevant Parts of the Diff

```diff
diff --git a/src/aleph/sdk/client/authenticated_http.py b/src/aleph/sdk/client/authenticated_http.py
index f84b97ca..6e1d39ad 100644
--- a/src/aleph/sdk/client/authenticated_http.py
+++ b/src/aleph/sdk/client/authenticated_http.py
@@ -540,15 +540,18 @@ async def create_instance(
         # Default to the QEMU hypervisor for instances.
         selected_hypervisor: HypervisorType = hypervisor or HypervisorType.qemu
 
+        environment_kwargs = dict(
+            internet=internet,
+            aleph_api=aleph_api,
+            hypervisor=selected_hypervisor,
+        )
+        if trusted_execution:
+            environment_kwargs["trusted_execution"] = trusted_execution
+
         content = InstanceContent(
             address=address,
             allow_amend=allow_amend,
-            environment=InstanceEnvironment(
-                internet=internet,
-                aleph_api=aleph_api,
-                hypervisor=selected_hypervisor,
-                trusted_execution=trusted_execution,
-            ),
+            environment=InstanceEnvironment(**environment_kwargs),
             variables=environment_variables,
             resources=MachineResources(
                 vcpus=vcpus,

The changes made are primarily focused on refactoring the code to improve readability and maintainability. There are no substantial modifications to the logic, and the addition of environment_kwargs does not introduce any significant risks. Therefore, the PR is categorized as 'BLACK' based on the potential impact on the codebase and review process.

@github-actions github-actions bot added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Aug 27, 2024
@philogicae philogicae requested a review from olethanh August 28, 2024 12:27
Copy link
Contributor

@olethanh olethanh left a comment

Choose a reason for hiding this comment

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

This doesn't change anything, if you pass trusted_execution=None or just don't pass it to InstanceEnvironment

You can test it by yourself by creating an InsatanceContent and using the method .dict(exclude_none=True)

@olethanh
Copy link
Contributor

Normally that bug with the trusted_execution was already fixed and the client already didn't send the trusted_execution field

@olethanh
Copy link
Contributor

In that client commit aleph-im/aleph-client@9eba48d

@philogicae
Copy link
Member Author

Canceled

@philogicae philogicae closed this Aug 28, 2024
@philogicae philogicae deleted the fix-optional-trust-exec branch August 28, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BLACK This PR has critical implications and must be reviewed by a senior engineer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants