ostreedev / ostree

Operating system and container binary deployment and upgrades
https://ostreedev.github.io/ostree/
Other
1.27k stars 291 forks source link

`ostree init` with no arguments prints error about `collection-id` key #2731

Open miabbott opened 1 year ago

miabbott commented 1 year ago

It's been a while since I've manually tried to do ostree init to create a new repo, so I blindly used the command as is and got an error message about a collection-id key:

[miabbott@frylock ~ ]$ mkdir repo
[miabbott@frylock ~ ]$ cd repo
[miabbott@frylock ~/repo ]$ ostree init 
error: Key file does not have key “collection-id” in group “core”

I would have expected that the command returns an error about a missing argument or something similar.

When I correctly used ostree init --repo=., the command completed successfully.

Using 2022.5:

$ rpm -q ostree
ostree-2022.5-2.fc36.x86_64
[miabbott@frylock ~/repo ]$ ostree --version
libostree:
 Version: '2022.5'
 Git: fcb959d012e5925196fc900e9665c3f8a0b708da
 Features:
  - libcurl
  - libsoup
  - gpgme
  - ex-fsverity
  - libarchive
  - selinux
  - openssl
  - libmount
  - systemd
  - release
  - p2p
dbnicholson commented 1 year ago

Oh, that's interesting. I suppose init should enforce the --repo option since that automatic detection of the current directory won't work on an uninitialized repo. In this case it ends up operating on /ostree/repo.

However, I do think there's a bug in ostree_repo_set_collection_id. If you pass in a collection ID of NULL, it calls g_key_file_remove_key to remove the core.collection_id key. That causes an error if the repo doesn't have a collection ID set, but I think it should gracefully handle that since the repo is already in the desired state.

dbnicholson commented 1 year ago

Oh, another sorta bug. If no --repo is passed, the ostree CLI tries to open it even when the subcommand says not to. Although, making it honor that makes it silently succeed if you already have an initialized /ostree/repo.

Yet another related papercut - ostree_repo_create succeeds if the repo is already initialized even if it's not writable by the caller. I feel like the create APIs should check OstreeRepo::writable after opening the repo.

But I think the correct thing to do here is to require init to have a --repo option. The way the code works now is that a --repo option is only required if neither the current directory nor /ostree/repo are initialized repositories.

miabbott commented 1 year ago

Oh, that's interesting. I suppose init should enforce the --repo option since that automatic detection of the current directory won't work on an uninitialized repo. In this case it ends up operating on /ostree/repo.

Ah that makes sense; I was doing this on a Fedora Silverblue system, so it was operating on /ostree/repo.

Repeating the test in a F36 container, the command errors out because of the missing --repo option:

[root@5a5354f4d6b2 ~]# ostree init
Usage:
  ostree init [OPTION…]

Initialize a new empty repository

Help Options:
  -h, --help                        Show help options

Application Options:
  --repo=PATH                       Path to OSTree repository (defaults to current directory or /sysroot/ostree/repo)
  --mode                            Initialize repository in given mode (bare, bare-user, bare-user-only, archive)
  --collection-id=COLLECTION-ID     Globally unique ID for this repository as an collection of refs for redistribution to other repositories
  -v, --verbose                     Print debug information during command processing
  --version                         Print version information and exit

error: Command requires a --repo argument

But I think the correct thing to do here is to require init to have a --repo option. The way the code works now is that a --repo option is only required if neither the current directory nor /ostree/repo are initialized repositories.

Sounds like a good improvement to me!