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

Fix optional trusted_exec #158

Closed
wants to merge 1 commit into from
Closed

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.

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
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