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

Removing `virsh` dependency #228

Merged

lukasfrank merged 8 commits into main from feature/remove-dependency-from-virsh

Mar 19, 2024

Contributor

so-sahu commented Mar 8, 2024

Proposed Changes

Using Pseudo-Terminal (pty) for bi-directional communication
Created development setup document and mentioned about the needed access.

Fixes #71

so-sahu added the integration-tests label

so-sahu self-assigned this

so-sahu requested a review from a team as a code owner

March 8, 2024 04:25

github-actions bot added size/L documentation enhancement server labels

lukas016 reviewed

View reviewed changes

docs/development/setup.md Outdated Show resolved Hide resolved

lukas016 reviewed

View reviewed changes

docs/development/setup.md Outdated Show resolved Hide resolved

lukas016 reviewed

View reviewed changes

provider/server/exec.go Outdated Show resolved Hide resolved

lukas016 reviewed

View reviewed changes

docs/development/setup.md Outdated Show resolved Hide resolved

lukas016 reviewed

View reviewed changes

provider/server/exec.go Show resolved Hide resolved

lukas016 requested changes

View reviewed changes

Contributor

lukas016 left a comment

code can be written more secure

Contributor Author

so-sahu commented Mar 8, 2024

code can be written more secure

Could you please provide more context here.

so-sahu marked this pull request as draft

March 8, 2024 09:56

lukas016 reviewed

View reviewed changes

provider/server/exec.go Show resolved Hide resolved

so-sahu marked this pull request as ready for review

March 8, 2024 12:20

so-sahu marked this pull request as draft

March 8, 2024 12:24

so-sahu marked this pull request as ready for review

March 8, 2024 12:46

so-sahu requested a review from lukas016

March 8, 2024 12:46

lukas016 reviewed

View reviewed changes

docs/development/setup.md Outdated Show resolved Hide resolved

lukas016 reviewed

View reviewed changes

provider/server/exec.go Outdated Show resolved Hide resolved

lukas016 reviewed

View reviewed changes

provider/server/exec.go Outdated Show resolved Hide resolved

so-sahu requested a review from lukas016

March 11, 2024 08:53

lukas016 reviewed

View reviewed changes

docs/development/setup.md Outdated Show resolved Hide resolved

lukas016 reviewed

View reviewed changes

provider/server/exec.go Outdated Show resolved Hide resolved

lukas016 requested changes

View reviewed changes

Contributor

lukas016 left a comment

concurrency safety can be improved.

so-sahu requested a review from lukas016

March 11, 2024 09:44

lukas016 previously approved these changes

View reviewed changes

Contributor

lukas016 left a comment

LGTM

Contributor

lukas016 commented Mar 11, 2024

Did you fix run of integration tests?

so-sahu force-pushed the feature/remove-dependency-from-virsh branch 4 times, most recently from 5493e66 to 9d565d4 Compare

March 12, 2024 08:42

Contributor

lukas016 commented Mar 12, 2024

@so-sahu could you pls study this: https://github.com/mattn/go-tty/blob/master/tty_unix.go#L22
I am interesting why they open same file with 2 file descriptors. I think it can have good reason. You don't have to solve it in this PR but maybe we can create follow up issue for improvement.

so-sahu marked this pull request as ready for review

March 12, 2024 10:27

so-sahu force-pushed the feature/remove-dependency-from-virsh branch from 9d565d4 to 531b8fe Compare

March 12, 2024 10:29

lukas016 reviewed

View reviewed changes

provider/server/exec.go Show resolved Hide resolved

lukas016 previously approved these changes

View reviewed changes

Contributor

lukas016 left a comment

LGTM

so-sahu dismissed lukas016’s stale review via

75beba7

March 15, 2024 14:04

so-sahu force-pushed the feature/remove-dependency-from-virsh branch 2 times, most recently from df4752d to d5be952 Compare

March 15, 2024 14:13

lukas016 approved these changes

View reviewed changes

Contributor

lukas016 left a comment

LGTM

so-sahu requested a review from lukasfrank

March 15, 2024 14:44

lukasfrank approved these changes

View reviewed changes

lukasfrank force-pushed the feature/remove-dependency-from-virsh branch 2 times, most recently from 8d3cc26 to 791229b Compare

March 19, 2024 08:14

lukasfrank enabled auto-merge (squash)

March 19, 2024 08:15

lukasfrank changed the title ~~Removing Dependency from Virsh~~ Removing virsh dependency

so-sahu added 8 commits

March 19, 2024 08:56


          removing dependency from virsh

7f32b4d


          changes for review comments

87bb30c


          adding protection for multiple connection at the same time to the sam…

566e70d

…e machine


          removing VirshExecutable

3d47580


          improving concurrency safety by using loadOrStore method

d61086e


          fixing integration tests permission issue

75468ef


          adding sync map to server

b04b24f


          updating document

ecff765

so-sahu force-pushed the feature/remove-dependency-from-virsh branch from 791229b to ecff765 Compare

March 19, 2024 08:57

lukasfrank merged commit 1c02cd9 into main

8 of 9 checks passed

lukasfrank deleted the feature/remove-dependency-from-virsh branch

March 19, 2024 08:58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation enhancement integration-tests server size/L