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

Proper reuse of control interface #251

Merged
merged 28 commits into from
Apr 25, 2024
Merged

Conversation

lingnoi
Copy link
Contributor

@lingnoi lingnoi commented Apr 23, 2024

Issues: #222

Definition of Done

The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.

@lingnoi lingnoi added the bug Something isn't working. Issue will appear in the change log "Bug Fixes" label Apr 23, 2024
@lingnoi lingnoi self-assigned this Apr 23, 2024
@krucod3 krucod3 self-requested a review April 24, 2024 08:23
@lingnoi lingnoi modified the milestone: v0.3.1 Apr 24, 2024
Copy link
Contributor

@krucod3 krucod3 left a comment

Choose a reason for hiding this comment

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

Looks good in general, I just had a couple of remarks regarding readability.

agent/src/runtime_connectors/runtime_facade.rs Outdated Show resolved Hide resolved
agent/src/control_interface/pipes_channel_context_info.rs Outdated Show resolved Hide resolved
agent/src/control_interface/pipes_channel_context_info.rs Outdated Show resolved Hide resolved
agent/src/runtime_manager.rs Outdated Show resolved Hide resolved
agent/src/workload.rs Outdated Show resolved Hide resolved
agent/src/workload.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@krucod3 krucod3 left a comment

Choose a reason for hiding this comment

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

Looks good now, just a small suggestion about the comparison.

agent/src/workload.rs Outdated Show resolved Hide resolved
agent/src/workload.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@krucod3 krucod3 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@krucod3 krucod3 merged commit 7facd55 into main Apr 25, 2024
7 checks passed
@krucod3 krucod3 deleted the 222_proper_reuse_of_control_interface branch April 25, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. Issue will appear in the change log "Bug Fixes" ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants