Add share-log subcommand to upload install.log to paste.rs#4511
Add share-log subcommand to upload install.log to paste.rs#4511Softer wants to merge 8 commits intoarchlinux:masterfrom
Conversation
|
Just note that I started a conversation about an arch official channel for posting these logs. I created a ticket to track the progress on gitlab@archlinux #833 |
|
We can change --share-log behavior transparently for users any time we need without changing user's expectation. :) |
|
Changes per review:
|
|
|
||
|
|
||
| def share_install_log() -> int: | ||
| from archinstall.lib.command import SysCommand |
There was a problem hiding this comment.
Why are they not global?
There was a problem hiding this comment.
Circular import: command.py imports debug, error, logger from output.py, so a top-level from archinstall.lib.command import SysCommand here creates a cycle. Removed in the latest commit anyway - switched to urllib.request so these imports are no longer needed.
| header += 'Continue?' | ||
|
|
||
| try: | ||
| from archinstall.tui.ui.components import tui |
There was a problem hiding this comment.
Why is this not global?
There was a problem hiding this comment.
Circular import: tui.ui.components imports debug from output.py, so a top-level import here creates a cycle. Has to stay local.
| from archinstall.tui.ui.components import tui | ||
|
|
||
| confirmed: bool = tui.run(lambda: _confirm_share(header)) | ||
| except Exception: |
There was a problem hiding this comment.
why do we need a try here?
| info('Cancelled.') | ||
| return 1 | ||
|
|
||
| import tempfile |
| import tempfile | ||
|
|
||
| if size > _PASTE_MAX_SIZE: | ||
| fd, tmp_path_str = tempfile.mkstemp(suffix='.log') |
There was a problem hiding this comment.
we can use pathlib here as it's the main pattern in the code base
if size > _PASTE_MAX_SIZE:
with tempfile.TemporaryDirectory() as tmp_dir:
upload_path = Path(tmp_dir) / 'archinstall_log_upload.log'
upload_path.write_bytes(content)
...
else:
...
In addition lets not call curl from syscommand but actually use Python build ins
try:
req = urllib.request.Request(_PASTE_URL, data=content)
with urllib.request.urlopen(req) as response:
url = response.read().decode().strip()
except urllib.error.URLError as e:
info(f'Upload failed: {e}')
return 1
| return 0 | ||
|
|
||
|
|
||
| async def _confirm_share(header: str) -> bool: |
There was a problem hiding this comment.
The TUI code should not be part of this file as archinstall can be used as a lib it should only expose the share function to consumers, but the consumers should be able to call it and use it independently from the TUI logic.
You can move the confirmation out to the main.py where the function is called, and after confirming it will just call the share function which will then run without interruption
There was a problem hiding this comment.
I see the point about lib purity, but share_install_log is specifically a CLI subcommand helper - it's invoked from main.py as archinstall share-log and there's no realistic scenario where someone calls it programmatically as a library function. Moving the confirmation out would split a 20-line function into two halves across two files for a separation that nobody will consume. Happy to do it if you feel strongly.
There was a problem hiding this comment.
It's about dependency management and footprint, this makes output.py dependent on the tui module which is not necessary.
you create a new public function for sharing the log file, so anyone using archinstall as a lib or otherwise would be able to use it, regardless if they need to or not. If anyone wants to create their own ISOs and use archinstall they are dependent on this hardcoded implementation of using the tui.
Also it makes isolated functionality testing very difficult as now a test needs to mock a full blown tui instead of being able to test the actual sharing logic in an isolated way.
In addtion, these
_PASTE_URL = 'https://paste.rs'
_PASTE_MAX_SIZE = 10 * 1024 * 1024
should be parameters into the function as well so the function can be tested easily by providing a simple mock url.
There was a problem hiding this comment.
You know better. Decoupled - share_install_log now takes paste_url, max_size, and confirm as parameters, zero TUI imports in output.py. The TUI confirmation lives in main.py as _tui_confirm and is passed via callback.
Per review: use stdlib urllib instead of shelling out to curl, drop unnecessary try/except around TUI confirmation, remove tempfile (content is passed directly as bytes).
|
Since share_install_log is now a standalone function in output.py with no TUI dependencies, I figured it would be a good candidate for unit tests. :-) Added tests/test_share_log.py covering all code paths:
|
| * To upload the log from the ISO image and get a shareable URL, run<br> | ||
| ```shell | ||
| curl -F'file=@/var/log/archinstall/install.log' https://0x0.st | ||
| archinstall --share-log |
There was a problem hiding this comment.
update to
archinstall share-log
| @@ -0,0 +1,94 @@ | |||
| # pylint: disable=redefined-outer-name | |||
There was a problem hiding this comment.
Thank you for adding tests!
|
|
||
| .. tip:: | ||
| | An easy way to submit logs is ``curl -F 'file=@/var/log/archinstall/install.log' https://0x0.st``. | ||
| | An easy way to submit logs is ``archinstall --share-log``, which uploads ``install.log`` to paste.rs and prints a shareable URL. |
There was a problem hiding this comment.
update to
archinstall share-log
| you can easily post the installation log using the following command: | ||
| `curl -F'file=@/var/log/archinstall/install.log' https://0x0.st` | ||
| you can easily upload the installation log and get a shareable URL by running: | ||
| `archinstall --share-log` |
Replaces the broken curl ... https://0x0.st hint with an integrated archinstall share-log command that uploads /var/log/archinstall/install.log to paste.rs and prints a shareable URL. Also updates the same hint in the README, the bug report docs, and the GitHub bug issue template, so all four user-facing references now point to the working command.
Refs #4396 (0x0.st has disabled uploads). Supersedes #4507, which removes the hint without offering a replacement.
Why paste.rs
The endpoint is referenced exactly once - a constant in archinstall/lib/output.py. If a self-hosted paste service is set up later, or 0x0.st returns, swapping it is a one-line change.
Behavior
$ archinstall share-log
About to upload /var/log/archinstall/install.log (12345 bytes) to https://paste.rs
The log may contain hostname, mirror URLs, package list and partition layout.
The uploaded paste is public.
Continue?
https://paste.rs/abc.def
Test plan