[Merge] ~cpete/ubuntu/+source/apport:update-udb-hook into ~ubuntu-core-dev/ubuntu/+source/apport:ubuntu/devel

[Benjamin Drung]

Review: Needs Fixing

I have some smaller remarks.

The commit "u-d-b: refactor log collection" refactors the code but also changes the logic (attach_file_if_exists vs attach_root_command_outputs). I would expect two commits instead (one for the refactoring and one for running the commands as root).

Diff comments:

> diff --git a/debian/package-hooks/ubuntu-desktop-bootstrap.py b/debian/package-hooks/ubuntu-desktop-bootstrap.py
> index 4e0eabc..0e1bd09 100644
> --- a/debian/package-hooks/ubuntu-desktop-bootstrap.py
> +++ b/debian/package-hooks/ubuntu-desktop-bootstrap.py
> @@ -28,20 +41,45 @@ def add_info(report, unused_ui):
>  }
>  """
>  
> -    subiquitylog = os.path.realpath("/var/log/installer/subiquity-server-debug.log")
> -    if os.path.exists(subiquitylog):
> -        report["SubiquityLog"] = hookutils.root_command_output(["cat", subiquitylog])
> +    # Check if the snap was updated
> +    # TODO: Add logic to support this outside of the live environment.
> +    #       It may be possible someone wants to report a bug against the
> +    #       installer after first boot.
> +    report["SnapUpdated"] = str(os.path.exists("/run/subiquity/updating"))
> +
> +    prefix = Path("/var/log/installer/")
> +    curtin_dir = prefix / "curtin-install/"
> +    log_map = {
> +        "SubiquityLog": prefix / "subiquity-server-debug.log",
> +        "CurtinLog": prefix / "curtin-install.log",
> +        "CurtinError.tar": prefix / "curtin-errors.tar",
> +        "ProbeData": prefix / "probe-data.json",
> +        "UdbLog": prefix / "ubuntu_bootstrap.log",
> +        "SubiquityTraceback": prefix / "subiquity-traceback.txt",
> +        "CurtinAptConfig": prefix / "curtin-install/subiquity-curtin-apt.conf",
> +        "CurtinCurthooksConfig": curtin_dir / "subiquity-curthooks.conf",
> +        "CurtinExtractConfig": curtin_dir / "subiquity-extract.conf",
> +        "CurtinInitialConfig": curtin_dir / "subiquity-initial.conf",
> +        "CurtinPartitioningConfig": curtin_dir / "subiquity-partitioning.conf",
> +    }
> +
> +    attach_journal(report, log_map)
> +
> +    hookutils.attach_hardware(report)
>  
> -    hookutils.attach_file_if_exists(
> -        report, "/var/log/installer/curtin-install.log", "CurtinLog"
> -    )
> -    hookutils.attach_file_if_exists(
> -        report, "/var/log/installer/curtin-errors.tar", "CurtinError.tar"
> -    )
> +    # Attach logs if they exist
> +    # The conventional way to attach logs would be to use the
> +    # hookutils.attach_file_if_exists method, but since subiquity logs
> +    # are mostly locked down with root r/w only then we will get a permission
> +    # error if the caller does not have permissions. Ask for elevated
> +    # permissions instead of requiring users know to run with sudo or similar
> +    command_mapping = {}
> +    for name, path in log_map.items():
> +        if os.path.exists(path):

Why not use pathlib functionality? path.exists()

> +            real_path = os.path.realpath(path)
> +            command_mapping[name] = f"cat {real_path}"

Why not use pathlib functionality? path.resolve()

>  
> -    hookutils.attach_file_if_exists(
> -        report, "/var/log/installer/block/probe-data.json", "ProbeData"
> -    )
> +    hookutils.attach_root_command_outputs(report, command_mapping)
>  
>      # Always set reports to private since we might collect sensitive data
>      report["LaunchpadPrivate"] = "1"


-- 
https://code.launchpad.net/~cpete/ubuntu/+source/apport/+git/apport/+merge/478129
Your team Ubuntu Core Development Team is subscribed to branch ~ubuntu-core-dev/ubuntu/+source/apport:ubuntu/devel.