Skip to content

reporegistry: introduce new distro.aliases file#1036

Closed
mvo5 wants to merge 2 commits intoosbuild:mainfrom
mvo5:distro-aliases-file
Closed

reporegistry: introduce new distro.aliases file#1036
mvo5 wants to merge 2 commits intoosbuild:mainfrom
mvo5:distro-aliases-file

Conversation

@mvo5
Copy link
Copy Markdown
Contributor

@mvo5 mvo5 commented Nov 12, 2024

This commit introduces a new distro.aliases file that replaces the
role of the existing symlinks to provide an alias name for a distro.
The reason is that when using go:embed symlinks are not supported
and we have currently centos-9.json -> centos-stream-9.json in
our osbuild-comopser distro definitions.

By moving them from symlinks to alias files we can start using
go:embed which is nice for the daemonless work, i.e. the
image-builder binary will be self-contained and one can do:

$ go run github.com/osbuild/image-builder/cmd/image-builder --list-images

directly without any further configuration. It also solves how to
keep the default repository configuration between osbuild-composer and
image-builder-cli in sync.


reporegistry: remove duplication in LoadRepositories()

This commit removes some directory traversal duplication from
LoadRepositories() on the expense of more "expensive" repo loading.
The new code will just always load all repos and return the one
matching the distro. The advantage of this approach is that when
aliases are introduced there is only a single code path that needs
changing.

mvo5 added 2 commits November 12, 2024 11:46
This commit removes some directory traversal duplication from
LoadRepositories() on the expense of more "expensive" repo loading.
The new code will just always load all repos and return the one
matching the distro. The advantage of this approach is that when
aliases are introduced there is only a single code path that needs
changing.
This commit introduces a new `distro.aliases` file that replaces the
role of the existing symlinks to provide an alias name for a distro.
The reason is that when using `go:embed` symlinks are not supported
and we have currently `centos-9.json -> centos-stream-9.json` in
our `osbuild-comopser` distro definitions.

By moving them from symlinks to alias files we can start using
`go:embed` which is nice for the daemonless work, i.e. the
`image-builder` binary will be self-contained and one can do:
```
$ go run github.com/osbuild/image-builder/cmd/image-builder --list-images
```
directly without any further configuration. It also solves how to
keep the default repository configuration between `osbuild-composer` and
`image-builder-cli` in sync.
@mvo5 mvo5 requested a review from thozza November 12, 2024 12:01
}
defer f.Close()
var aliases []struct {
Name string `json:"name"`
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too happy with the names, ideas for better naming welcome here :)

@thozza
Copy link
Copy Markdown
Member

thozza commented Nov 12, 2024

Just a quick comment: I am not sure if distro.aliases is the best name since we already have distro aliases, and it is a different thing:

  • // RegisterAliases configures the factory with aliases for distro names.
    // The provided aliases map has the following constraints:
    // - An alias must not mask an existing distro.
    // - An alias target must map to an existing distro.
    func (f *Factory) RegisterAliases(aliases map[string]string) error {
    var errors []string
    for alias, target := range aliases {
    var targetExists bool
    for _, factory := range f.factories {
    if factory(alias) != nil {
    errors = append(errors, fmt.Sprintf("alias '%s' masks an existing distro", alias))
    }
    if factory(target) != nil {
    targetExists = true
    }
    }
    if !targetExists {
    errors = append(errors, fmt.Sprintf("alias '%s' targets a non-existing distro '%s'", alias, target))
    }
    }
    // NB: iterating over a map of aliases is not deterministic, so sort the
    // errors to make the output deterministic
    sort.Strings(errors)
    if len(errors) > 0 {
    return fmt.Errorf("invalid aliases: %q", errors)
    }
    f.aliases = aliases
    return nil
    }
  • https://github.com/osbuild/osbuild-composer/blob/2a5d25d9c02e29a8898166f1e1cdc9c58313e78c/cmd/osbuild-composer/config.go#L18

From my PoV, the ability to add a new repo config file or a symlink is a feature. I have yet to look at the code, but I would keep the option to redefine the alias for the user (ideally, not compiled into the binary).

@mvo5
Copy link
Copy Markdown
Contributor Author

mvo5 commented Nov 12, 2024

Just a quick comment: I am not sure if distro.aliases is the best name since we already have distro aliases, and it is a different thing:
[..]

Oh, this super interesting - the only reason I am adding this is to avoid the symlink centos-9.json -> centos-stream-9.json in the default repo configuration. If a distro_alias in the composer config fixes this equally nicely and we can drop the symlink none of the distro.alises support is needed.

From my PoV, the ability to add a new repo config file or a symlink is a feature. I have yet to look at the code, but I would keep the option to redefine the alias for the user (ideally, not compiled into the binary).

Both the ability to add new repo config, symlinks and to mask existing repos would be preserved. The goal of what I'm doing is strictly about making it easier to share the default/canonical repos from composer across multiple projects (image-builder-cli in my specific case).

@mvo5 mvo5 marked this pull request as draft November 12, 2024 15:01
@mvo5
Copy link
Copy Markdown
Contributor Author

mvo5 commented Nov 12, 2024

Moving back to "draft" as it seems people do not really like this idea very much so far. So I will think a bit more what else we can do.

@mvo5
Copy link
Copy Markdown
Contributor Author

mvo5 commented Nov 12, 2024

Closing in favor of #1038 that hopefully addresses the concerns here.

@mvo5 mvo5 closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants