-
Notifications
You must be signed in to change notification settings - Fork 262
Add download all and all namespaces flags #1917
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
base: main
Are you sure you want to change the base?
Conversation
| downloadServices := make(map[string]*DownloadService) | ||
|
|
||
| // Collect all files in memory | ||
| allFiles := make(map[string][]byte) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this cause memory issues?
If captures files are large and there are multiple in different namespaces
| if downloadAllNamespaces { | ||
| fmt.Println("Downloading all captures from all namespaces...") | ||
| } else { | ||
| fmt.Println("Downloading all captures...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - add 'for namespace x' here
| return nil | ||
| } | ||
|
|
||
| func downloadAllCaptures(ctx context.Context, config *rest.Config, namespace string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
recommend breaking this down into a couple functions,
| } | ||
|
|
||
| // Create the final archive | ||
| timestamp := time.Now().Format("20060102150405") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a magic number - there should be constant for this in the time package
|
|
||
| downloadCapture.Flags().StringVar(&blobURL, "blob-url", "", "Blob URL from which to download") | ||
| downloadCapture.Flags().StringVar(&captureName, "name", "", "The name of a the capture") | ||
| downloadCapture.Flags().BoolVar(&downloadAll, "all", false, "Download all available captures") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit unclear but looks like this is download all for specific namespace
We should clarify that so it's apparent the different between all and all-namespaces
Description
Allow user to download all captures from a specific namespace or from all namespaces
Related Issue
#1836
Checklist
git commit -S -s ...). See this documentation on signing commits.Screenshots (if applicable) or Testing Completed
Please add any relevant screenshots or GIFs to showcase the changes made.
Additional Notes
Add any additional notes or context about the pull request here.
Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.