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: go-resty component download #1466

Merged
merged 10 commits into from
Dec 6, 2023
Merged

Conversation

jon-stewart
Copy link
Contributor

@jon-stewart jon-stewart commented Nov 29, 2023

Summary

  • Using go-resty
  • Download progress
    • progress shown in CLI spinner
    • prototype components show progress in MB (we do not have access to total file size)
    • v1 components show process in %
    • CLI spinner updated if value changes
  • No request timeout
    • can be set with environmental variable CDK_DOWNLOAD_TIMEOUT_MINUTES

How did you test this change?

Unit tests + manual

Screen.Recording.2023-12-04.at.11.25.45.mov

Issue

https://lacework.atlassian.net/browse/GROW-2596

@jon-stewart jon-stewart self-assigned this Nov 29, 2023
@jon-stewart jon-stewart requested a review from a team as a code owner November 29, 2023 12:49
lwcomponent/http.go Outdated Show resolved Hide resolved
@jvogt
Copy link

jvogt commented Nov 29, 2023

Is the 5 minute timeout a hard limit? My expectation is the download will not timeout as long as there is progress.

Example: I was on conference wifi, roughly 3mbit, and it took about 15 minutes to download the sast component manually with curl.

I would expect a sane timeout if the download does not start (something like 15 seconds), no timeout if there is progress, and maybe a longer timeout (1-5 minutes) if no data has been received.

lwcomponent/http.go Outdated Show resolved Hide resolved
}

if totalSize == 0 {
fmt.Printf("Downloaded: %.0fkb\n", float64(size)/(1<<10))
Copy link
Contributor

Choose a reason for hiding this comment

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

The lwcomponent should not interact with STDOUT or STDERR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should we indicate progress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am now using a virtual terminal - is this the approach you had in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afiune Moved the code to the CLI package and showing progress in the spinner suffix

@jon-stewart
Copy link
Contributor Author

Is the 5 minute timeout a hard limit? My expectation is the download will not timeout as long as there is progress.

Example: I was on conference wifi, roughly 3mbit, and it took about 15 minutes to download the sast component manually with curl.

I would expect a sane timeout if the download does not start (something like 15 seconds), no timeout if there is progress, and maybe a longer timeout (1-5 minutes) if no data has been received.

15 minutes! Ok yeah, we can't have a hard timeout then - unless configured with an environmental variable.

The timeouts you describe are handled by default, those are connection timeout and idle connection timeout.

lwcomponent/http.go Outdated Show resolved Hide resolved
@jon-stewart jon-stewart force-pushed the jon-stewart/GROW-2596/go-resty branch from 4ea5ee8 to de9304e Compare December 4, 2023 11:23
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

@jon-stewart jon-stewart merged commit efba629 into main Dec 6, 2023
1 check passed
@jon-stewart jon-stewart deleted the jon-stewart/GROW-2596/go-resty branch December 6, 2023 16:55
This was referenced Dec 6, 2023
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.

5 participants