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

feat: add exec command args for xlinectl lock command #846

Closed
wants to merge 4 commits into from

Conversation

bhavik-goplani
Copy link

@bhavik-goplani bhavik-goplani commented Jun 10, 2024

Please briefly answer these questions:

  • what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)
    [Feature]: Add exec command args for xlinectl lock command #823

  • what changes does this pull request make?
    Added exec command support to xlinectl lock command. Updated the README with new examples for lock.

  • are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)
    No

Additional comments:
I'm not sure how to test the exec command args. I thought of two solutions:

  1. Extend LockRequest or Test Case Representation: Add a way to represent the exec commands that should be executed after the lock is acquired. This could be a new field in the LockRequest struct or a separate structure linked to the test case.
  2. Modify the Test Case: Adjust the test case in macros.rs to include the exec commands as part of the expected outcome.

@mergify mergify bot requested a review from a team June 10, 2024 18:15
Signed-off-by: bhavik-goplani <56516858+bhavik-goplani@users.noreply.github.com>
Signed-off-by: bhavik-goplani <56516858+bhavik-goplani@users.noreply.github.com>
Signed-off-by: bhavik-goplani <56516858+bhavik-goplani@users.noreply.github.com>
Signed-off-by: bhavik-goplani <56516858+bhavik-goplani@users.noreply.github.com>
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 14 lines in your changes missing coverage. Please review.

Project coverage is 75.61%. Comparing base (e35b35a) to head (038bca5).
Report is 108 commits behind head on master.

Files Patch % Lines
crates/xlinectl/src/command/lock.rs 36.36% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #846      +/-   ##
==========================================
+ Coverage   75.55%   75.61%   +0.05%     
==========================================
  Files         180      187       +7     
  Lines       26938    27685     +747     
  Branches    26938    27685     +747     
==========================================
+ Hits        20353    20933     +580     
- Misses       5366     5465      +99     
- Partials     1219     1287      +68     

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

@CrystalAnalyst
Copy link
Collaborator

Sorry for the late response. I really appreciate your interest about Xline and the contribution you've made.
I've reviewd your code logic, it's natural and clear.
However, recently we did a major change about this xlinectl lock command, you can refer to the #820.
In essence, the lock usage is changed from : client.lock_client().lock(req).await?;
to : let mut xutex = Xutex::new().await?; let xutex_guard = xutex.lock_unsafe().await?
So you may update your repo and make corresponding change, especially the :
pub(crate) async fn execute(client: &mut Client, matches: &ArgMatches) -> Result<()> .
And for the test, we have a validation test in the: Xline/scripts/validation_test.sh, you can add your test case here.
If you have any questions, feel free to ask.

@bhavik-goplani
Copy link
Author

@CrystalAnalyst Thanks for the feedback. I'll close this pull request and create a new PR after pulling the recent changes.

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.

None yet

2 participants