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

use ssh to send image instead of http to avoid timeouts #1087

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rcooke-warwick
Copy link
Contributor

@rcooke-warwick rcooke-warwick commented Dec 13, 2023

Copy link
Member

@vipulgupta2048 vipulgupta2048 left a comment

Choose a reason for hiding this comment

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

Flashing looks so much cleaner and it makes sense. Comments inline

)
}

// note: for the qemu case, we are assuming a shared volume between core and worker - this means that any image we want to flash must be in this volume - is it in all our cases?
Copy link
Member

Choose a reason for hiding this comment

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

Who is this question for?
If you are asking me then yeah why not. The docker-compose will define the volumes once for QEMU and that should be the case everywhere no?

async () => {
this.logger.log(`Sending image ${imagePath}`);
try{
// arrives at worker as /data/os.img.gz
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// arrives at worker as /data/os.img.gz
// Sends image to worker at TARGET_PATH

);

let TARGET_PATH = imagePath;
// if using remote worker i.e autokit/testbot, send the image over ssh
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// if using remote worker i.e autokit/testbot, send the image over ssh
// if autokit/testbot, send the image over ssh

Clarity++, saying remote worker adds another shade of terminology

Comment on lines +128 to +150
// Wrap sending of image in a retry - hopefully the --partial arguement in the rsync command means it will resume
// in the case of an error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Wrap sending of image in a retry - hopefully the --partial arguement in the rsync command means it will resume
// in the case of an error
// Send image with rsync command, retry if error

Comment on lines +137 to +159
console.log(e);
throw new Error(`Rysnc error: ${e.message}`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(e);
throw new Error(`Rysnc error: ${e.message}`)
throw new Error(`Rysnc error: ${e}`)

},
);

let TARGET_PATH = imagePath;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let TARGET_PATH = imagePath;
let targetPath = imagePath;

Can we change this everywhere to follow the same naming conventions.

Copy link
Contributor

@jakogut jakogut left a comment

Choose a reason for hiding this comment

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

Nice improvement. Re-inventing SSH over HTTP was always a bit of a head scratcher.

Change-type: patch
Signed-off-by: Ryan Cooke <[email protected]>
Change-type: patch
Signed-off-by: Ryan Cooke <[email protected]>
Change-type: patch
Signed-off-by: Ryan Cooke <[email protected]>
Change-type: patch
Signed-off-by: Ryan Cooke <[email protected]>
Change-type: patch
Signed-off-by: Ryan Cooke <[email protected]>
Change-type: patch
Signed-off-by: Ryan Cooke <[email protected]>
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.

3 participants