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.

Sign up for GitHub

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

Jump to bottom

Feat/#61 remote exec #62

Merged
merged 26 commits into from
Oct 9, 2024
Merged

Feat/#61 remote exec #62

merged 26 commits into from
Oct 9, 2024

Conversation

comavius
Copy link
Member

@comavius comavius commented Oct 5, 2024

関連Issue

概要

telnetからsshに変更し実装

変更内容

  • tenlet moduleを削除
  • remote_exec moduleを作成
  • RemoteExec traitを実装
  • RemoteExecの実装としてSshConnectionを実装

チェックリスト

  • テストが通っている
  • 下記のいずれかを満たしている
      • 変更点についてテストを追加/修正した
      • テストを追加するissueを立てた
      • テストが不要な変更である
  • レビュワーを指定した
  • タグをつけた

補足

@comavius comavius added the enhancement New feature or request label Oct 5, 2024
@comavius comavius self-assigned this Oct 5, 2024
@comavius
Copy link
Member Author

comavius commented Oct 5, 2024

テストをするときにsystemctlでdockerデーモンを起動しているせいでcargo testするたびにroot権限を求められて面倒くさい

@comavius
Copy link
Member Author

comavius commented Oct 5, 2024

actionsの環境では最初からdockerデーモンが立ち上がっていそう

@comavius
Copy link
Member Author

comavius commented Oct 5, 2024 •
edited
Loading

ソケットがrunningではない場合systemctl status dockerが異常終了していそうなので解決できそう

@comavius
Copy link
Member Author

comavius commented Oct 5, 2024

正常系の失敗ケースは追加したいかも

@comavius
Copy link
Member Author

comavius commented Oct 5, 2024

あと、ジャッジサーバーの責任で起こるエラーと作問者の責任で起こるエラーがありそれらを適切にハンドリングする必要があるため、anyhow::Resultを使うべきではないかもしれない

@comavius
Copy link
Member Author

comavius commented Oct 6, 2024

timeoutが動いていない

@comavius
Copy link
Member Author

comavius commented Oct 6, 2024

あと、ジャッジサーバーの責任で起こるエラーと作問者の責任で起こるエラーがありそれらを適切にハンドリングする必要があるため、anyhow::Resultを使うべきではないかもしれない

解決した

kenken714
kenken714 reviewed Oct 7, 2024
View reviewed changes
Copy link
Collaborator

@kenken714 kenken714 left a comment

Choose a reason for hiding this comment

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

とりあえずclippyでwarningが出ていた分

judge-control-app/src/remote_exec/ssh.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh/tests.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh/tests.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh/tests.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh/tests.rs Outdated Show resolved Hide resolved
judge-control-app/src/remote_exec/ssh/tests.rs Outdated Show resolved Hide resolved
@kenken714
Copy link
Collaborator

手元でcargo clippyをしてからPRをしてください:pray:

@comavius
Copy link
Member Author

comavius commented Oct 7, 2024

cargo clippy --fixした

@comavius comavius requested a review from kenken714 October 7, 2024 14:46
@comavius
Copy link
Member Author

comavius commented Oct 7, 2024

@kenken714 clippyとcargo fmtで結果が変わる部分がありそう

kenken714
kenken714 approved these changes Oct 8, 2024
View reviewed changes
judge-control-app/src/remote_exec/ssh/tests.rs
)
.await;
stop_ssh_docker_container(uuid).await.unwrap();
assert!(resp.is_err_and(|e| match e {
Copy link
Collaborator

Choose a reason for hiding this comment

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

assert!(matches!(
        resp,
        Err(remote_exec::ssh::SshExecError::RemoteServerError(_))
    ));

だと良い気がするけど、任せます

@kenken714
Copy link
Collaborator

kenken714 commented Oct 8, 2024 •
edited
Loading

  • trait実装を確認
  • testが通ることを確認

@kenken714
Copy link
Collaborator

kenken714 commented Oct 8, 2024 •
edited
Loading

テストをするときにsystemctlでdockerデーモンを起動しているせいでcargo testするたびにroot権限を求められて面倒くさい

:koreni_natteru: (手元だけ)

@comavius
Copy link
Member Author

comavius commented Oct 8, 2024

(dockerデーモンが起動済み or root権限あり)が満たされないなら自動で落とすようにしたからよくはなっているはず

@comavius comavius merged commit 7e4522c into develop Oct 9, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenken714 kenken714 kenken714 approved these changes

@hayatroid hayatroid Awaiting requested review from hayatroid

Assignees

@comavius comavius

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

遠隔実行の実装
2 participants
@comavius @kenken714