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

Add support for agent status in handshake process #56

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

sophie-cluml
Copy link
Contributor

Closes: #55

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.12%. Comparing base (98676f3) to head (4b90217).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   70.48%   71.12%   +0.64%     
==========================================
  Files          10       10              
  Lines        1213     1240      +27     
==========================================
+ Hits          855      882      +27     
  Misses        358      358              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sophie-cluml sophie-cluml force-pushed the sophie/55 branch 2 times, most recently from 3effd61 to 1223d7c Compare January 15, 2025 07:39
src/types.rs Outdated
@@ -128,3 +128,27 @@ pub struct Tidb {
pub version: String,
pub patterns: Vec<TiRule>,
}

/// Corresponds to the review-database's `tables::agent::Status`
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there’s a conceptual issue with the current implementation. The Status enum defined in this PR is aligned with the Status in the review-database, but this correspondence doesn't seem appropriate in this context.

The Status in review-database is intended to represent the agent’s status from REview's perspective, while the status field in the handshake protocol is meant to allow the agent to report its own status. These are distinct concerns. The agent's status field should reflect only the states the agent can report about itself, and it shouldn’t overlap with REview’s internal representation or database schema.

As far as I understand, this new status field is to differentiate whether the agent is operating in normal mode or minimal mode. If this is still the case, the enum should directly represent those operational states. How the status is stored in the database should remain an implementation detail internal to REview and not be a concern for the agent.

Could you clarify and potentially revise the PR to align the Status enum with the agent's self-reported states rather than the database schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for very kind comment, the changes are applied.

@msk
Copy link
Contributor

msk commented Jan 19, 2025

@sehkone, I've got just one minor request for a change, mentioned in my review. Once that's addressed, this PR is good to merge. I'll leave it to you to merge. Feel free to release 0.9.0 if needed.

@sehkone
Copy link
Contributor

sehkone commented Jan 19, 2025

@sehkone, I've got just one minor request for a change, mentioned in my review. Once that's addressed, this PR is good to merge. I'll leave it to you to merge. Feel free to release 0.9.0 if needed.

Thanks for the review! Once the requested change is addressed, I'll handle merging the PR and releasing version 0.9.0.

@sophie-cluml sophie-cluml force-pushed the sophie/55 branch 2 times, most recently from 9e2666f to 8b55d4b Compare January 20, 2025 03:10
src/lib.rs Outdated
APP_NAME,
APP_VERSION,
PROTOCOL_VERSION,
Status::Normal,
Copy link
Contributor

@sehkone sehkone Jan 20, 2025

Choose a reason for hiding this comment

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

Since this method name doesn't look like it describes normal status, is Status::Normal relevant here?

Copy link
Contributor

Choose a reason for hiding this comment

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

While I may not have the full context, including corner cases, I feel Running serves as a better opposite to Idle than Normal.

Copy link
Contributor

@sehkone sehkone Jan 20, 2025

Choose a reason for hiding this comment

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

While I may not have the full context, including corner cases, I feel Running serves as a better opposite to Idle than Normal.

Only if the agent is really in the status of running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed in another channel, I modified it to Working, and it was clarified that handshake_incompatible_err is the name of a test method.

@sehkone
Copy link
Contributor

sehkone commented Jan 20, 2025

Status::Idle clearly describes its intended meaning. My concern is finding a term that properly represents the other status. While I suggested Running, it implies the agent is actively working, but it should also include the state where the agent is starting to fetch the configuration. For this reason, I am now suggesting Ready, as it encompasses both scenarios: when the agent is working and when it is preparing to work.

@sophie-cluml sophie-cluml force-pushed the sophie/55 branch 2 times, most recently from 5f9f287 to 5840b19 Compare January 20, 2025 06:35
@sophie-cluml
Copy link
Contributor Author

Changes ard done.

@sehkone
Copy link
Contributor

sehkone commented Jan 20, 2025

Since @msk's request has been addressed and he delegated the merge to me, I am merging this.

@sehkone sehkone merged commit a2186a5 into petabi:main Jan 20, 2025
6 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.

Allow Clients to Communicate Status During Handshake
3 participants