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
Serve a tarball containing the contents of a given directory. #53
Conversation
This will make it easier to get all logs for a given bug; preventing users needing to run scripts to download all files. - we cannot make the link exist in the directory listing as there are scripts that automate downloads which would pick this up. - Unsure if "?format=tar.gz" is the right option to enable this; I couldn't think of something easy to do but hard to not get picked up by existing automation, and wouldn't conflict with existing filenames.
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 generally sensible.
Suggestions:
- document this at https://github.com/matrix-org/rageshake#get-apilisting
- add a link to the tarball from the GH issue, so that there is a direct link for new rageshakes.
Another suggestion: use a completely different endpoint (eg |
…tter, add documentation.
I thought about this; my logic is that if you are looking at a directory listing, it's easier to add a ?format=tar.gz on the end than change endpoint. |
Oh, and explicitly not adding the tarball to the github issues for now, i want to get the ability to download working in production before we start scattering links to it (in case we do want to change it to /archive or whatever) |
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.
lgtm otherwise
log.Println("Serving directory listing of", path) | ||
http.ServeFile(w, r, path) | ||
return | ||
serveDirectory(w, r, path) | ||
} |
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 needs a return
? (or you need to guard the rest of it with an else
)
logserver.go
Outdated
} | ||
log.Println("Serving directory listing of", path) | ||
http.ServeFile(w, r, path) | ||
return |
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 return
is redundant
This will make it easier to get all logs for a given bug; preventing users
needing to run scripts to download all files.
we cannot make the link exist in the directory listing as there are scripts
that automate downloads which would pick this up.
Unsure if "?format=tar.gz" is the right option to enable this; I couldn't think
of something easy to do but hard to not get picked up by existing automation, and
wouldn't conflict with existing filenames. Suggestions welcome.