Feature/registry alias #273

dgehriger · 2024-11-21T18:31:00Z

This PR introduces a whole set of features which are all related to implement a comprehensive build workflow.

Important

This PR bumps the Proto.toml edition to 0.10.

buffrs --insecure ...
buffrs install --only-dependencies
buffrs install --generate-buf-yaml <file> [<prefix>]
Repository Configuration File (repo-root/.buffrs/config.toml)
Registry Aliases
Improved Path Dependency Handling
- Nested Path Dependencies
- Path Dependency and Registry Dependency
Code Refactoring
- CLI code moved to cli.rs

`buffrs --insecure ...`

The -k / --insecure general flag skips SSL validation during operations. Useful for environments with self-signed certificates or testing scenarios, such a Docker containers.

`buffrs install --only-dependencies`

Adding this flag during buffrs install (e.g., buffrs install --only-dependencies) installs only the vendor, while leaving a package's own proto files untouched.

`buffrs install --generate-buf-yaml <file> [<prefix>]`

Adding this flag during buffrs install (e.g., buffrs install --generate-buf-yaml) generates a buf.yaml file in the proto directory matching installed dependencies during dependency resolution. This enables seamless use of buf for linting and generating proxies/stubs from proto files.

The flag will either create a new buf.yaml file from scratch or update an existing one with the latest dependencies.
IDE extensions, such as the Buf Linter for Visual Studio Code, will readily pick up the buf.yaml file and provide linting capabilities. A typical buf.yaml file looks like this:

version: v2
modules:
  - path: "."
    excludes:
      - vendor
  - path: vendor/api-movie-player
  - path: vendor/api-robot-arm-motion-control
  - path: vendor/lib-geom
  - path: vendor/lib-waypoints
deps:
  - buf.build/googleapis/googleapis
  - buf.build/grpc/grpc
lint:
  use:
    - DEFAULT
  except:
    - PACKAGE_VERSION_SUFFIX
breaking:
  use:
    - FILE

Repository Configuration File (repo-root/.buffrs/config.toml)

A new configuration file, .buffrs/config.toml, can be added to a repository to specify various options for the buffrs tool. This file is optional and can be used to set default values for commands, such as buffrs install, or to specify registry aliases and a default registry.

Note

There was a lot of discussion about the location and name of the configuration file. In particular, it was suggested that it should be placed at $HOME/.config/buffrs.toml. It is true that there is a general move to collect configuration files in $HOME/.config/, but this does not apply to this case, because the purpose of the configuration file is to specify options for a specific repository. Therefore, it makes sense to place the configuration file in the root of the repository.

Further research provided no indication that a common configuration folder is used for repositories in a similar fashion as for $HOME directory. This was confirmed by Mara Schulke. Therefore, this PR keeps the file at its original location.

The configuration has the following structure:

edition = "0.10"

[registries]
acme = "https://conan.acme.com/artifactory"

[registry]
default = "acme"

[command]
default_args = ["--insecure"]

[commands.install]
default_args = ["--generate-buf-yaml"]

edition: Specifies the edition of the configuration file. This field is mandatory and must be set to the current edition of the configuration file, which mirrors the edition of the Proto.toml file.
registries: Specifies registry aliases. The key is the alias, and the value is the actual registry URL.
registry: Specifies the default registry to use when no registry is specified on the command line.
command: Specifies default arguments for the buffrs tool using the default_args field. The default_args field is an array of default arguments.
commands.<command>: Specifies default arguments for a specific command. The default_args field is an array of default arguments.

Registry Aliases

If configured in the configuration file, registry aliases can be used anywhere a registry URL is expected. This simplifies the use of registries by providing a human-readable alias instead of a full URL.

Aliases on the Command Line

Aliases can be used on the command line by specifying the --registry flag with the alias name. For example:

buffrs publish --registry acme --repository grpc

Aliases in Proto.toml

The registry field in Proto.toml now supports aliases. For instance:

edition = "0.10"

[package]
type = "api"
name = "grpc-test"
version = "0.1.0"

[dependencies.api-examples-hub]
version = "=0.2.0"
registry = "acme" # Alias for "https://conan.acme.com/artifactory"
repository = "grpc"

Alias resolution during `package` and `publish`

Buffrs resolves registry aliases when packing or publishing to an upstream repository. To ensure compatibility, the original Proto.toml is preserved as Proto.toml.orig, while the normalized version is used during operations. This mirrors Cargo's behavior.

Example:

Original Proto.toml:

edition = "0.10"

[package]
type = "api"
name = "api-examples-test"
version = "0.3.0"

[dependencies.lib-geom]
version = "=0.1.0"
registry = "acme"
repository = "grpc"

Generated Proto.toml:

# THIS FILE IS AUTOMATICALLY GENERATED BY BUFFRS
#
# When uploading packages to the registry buffrs will automatically
# "normalize" Proto.toml files for maximal compatibility
# with all versions of buffrs and also rewrite `path` dependencies
# to registry dependencies.
#
# If you are reading this file be aware that the original Proto.toml
# will likely look very different (and much more reasonable).
# See Proto.toml.orig for the original contents.
edition = "0.10"

[package]
type = "api"
name = "api-examples-test"
version = "0.3.0"

[dependencies.lib-geom]
version = "=0.1.0"
registry = "https://conan.acme.com/artifactory"
repository = "grpc"

Improved Path Dependency Handling

buffrs already supports path dependencies, e.g.

edition = "0.10"

[dependencies]
lib-geom = { path = "../lib-geom" }

This PR adds the following new features:

Nested Path Dependencies

Proto packages may now specify a path dependency that itself has path dependencies, improving fast turnaround times during development.

Path Dependency and Registry Dependency

Similar to Cargo, Proto packages may now specify a path dependency and a registry dependency for the same package. This is useful for development scenarios where a package references a local package, but, once published, needs to reference a registry package.

edition = "0.10"

[dependencies]
lib-geom = { path = "../lib-geom", version = "=0.1.0", repository = "grpc", registry = "acme" }

When publishing, the path dependency is dropped, and only the registry dependency is maintained during alias resolution. The original Proto.toml is preserved as Proto.toml.orig.

Even during development, the version constraint will be checked against version found at the local path.

Code Refactoring

CLI code moved to cli.rs

The CLI code formerly in main.rs has been moved to cli.rs, and and is invoked from main.rs through a cli::run() function. The main purpose of this change is to allow buffrs to be invoked from build.rs scripts simply by declaring buffrs as a dev-dependency in Cargo.toml, without having to install the binary itself.

Add .buffrs/config.toml registries and hierarchical packages

Bumps [gix-path](https://github.com/Byron/gitoxide) from 0.10.7 to 0.10.10. - [Release notes](https://github.com/Byron/gitoxide/releases) - [Changelog](https://github.com/Byron/gitoxide/blob/main/CHANGELOG.md) - [Commits](GitoxideLabs/gitoxide@gix-path-v0.10.7...gix-path-v0.10.10) --- updated-dependencies: - dependency-name: gix-path dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

# Conflicts: # Cargo.lock # Cargo.toml # src/main.rs # src/manifest.rs # src/registry/mod.rs # tests/cmd/publish/in # tests/cmd/publish/out

# Conflicts: # Cargo.lock

…kage # Conflicts: # Cargo.lock

dgehriger · 2024-11-29T22:26:30Z

@mara-schulke : some of the unit tests are failing due to the change in the manifest version (0.9 → 0.10). I'm fixing this now and will update.

mara-schulke · 2024-11-30T08:10:23Z

Hi this is great thank you so much! One note before I'd start a detailed review: The proto rust module is not necessary as this is covered by Tonic itself:

https://docs.rs/tonic-build/latest/tonic_build/struct.Config.html#method.include_file

Could you maybe revert that command?

Once done I'm happy to review and merge this asap!

src/main.rs

src/package/compressed.rs

src/registry/artifactory.rs

src/registry/mod.rs

mara-schulke · 2024-12-01T12:16:51Z

Im not entirely done with reviewing but I've commented some points of concern above so they can be already addressed

mara-schulke · 2024-12-01T12:20:32Z

You should not invoke the CLI level code in a build.rs as doing network requests as part of your build process is going to yield you flaky builds

I'm not keen on re introducing this pattern (this was actually once possible with buffrs - it turned out to be causing all sorts of issues). Scheduling the installation step of your protos is better done by using (or better integrating buffrs into) a proper build system for your project like nix, bazel etc.

We have done this at Helsing and it works great, incl caching etc.

…lias

dgehriger · 2024-12-01T17:59:36Z

One note before I'd start a detailed review: The proto rust module is not necessary as this is covered by Tonic itself:

docs.rs/tonic-build/latest/tonic_build/struct.Config.html#method.include_file

Could you maybe revert that command?

What a joke... I wasn't aware of this. Of course. a5651c1

mara-schulke

This is definitely going in the right direction although I feel like this is loosening the type safety and semantic guarantees we currently provide. I'm happy to have another chat about this to get it over the line!

src/command.rs

src/config.rs

mara-schulke · 2024-12-05T23:03:37Z

src/config.rs

+    config_path: Option<PathBuf>,
+
+    /// Default registry alias to use if none is specified
+    default_registry: Option<String>,


Last new type request: Could we use a registry name new type to ensure that this string is infact a valid registry name?

I would suggest using a similar convention as with package names

You could basically derive / share a lot of logic with the PackageName type

mara-schulke · 2024-12-05T23:04:47Z

src/config.rs

+    /// - None -> alias://<default>
+    pub fn parse_registry_arg(&self, registry: &Option<String>) -> miette::Result<RegistryRef> {
+        match registry {
+            Some(registry) => RegistryRef::from_str(registry),


Nit: .parse is more idiomatic imo

mara-schulke · 2024-12-05T23:07:07Z

src/config.rs

+}
+
+impl Config {
+    /// Create a new configuration with default values


This comment lies

Could you roughly follow the public api semantics from the manifest / cache?

Can you give me specifics?

Ah yes sorry.

So this function will do IO although the comment says it will create a new one with default values!

With regards to the common pattern I was referring to the Manifest struct for example

mara-schulke · 2024-12-05T23:40:09Z

src/registry/mod.rs

+    /// A URL to a registry
+    Url(RegistryUri),
+    /// An alias to a registry
+    Alias(String),


In my understand you have two types:

RegistryRef (a reference, potentially aliased registry) and RegistryUri the uri version, independent of its origin

You mean we could express the Alias as Alias(String, Option<RegistryUri>) ?

I found it safer to not use the option, but a third type. But for aliases, we need to remember the alias name and URL at all times, as, depending on the context, we do need to be able to access the alias, even if resolved.

What could be done is to split this enum into two types:

enum RawRegistryRef { Url(RegistryUri), Alias(String), }; enum RegistryRef { Url(RegistryUri), Alias(String, RegistryUri), };

The former is returned just after parsing the config files / command lines, and the latter is the result of resolving the aliases.

Well I'm talking about:

enum RegistryRef { Uri(Uri) Alias(RegistryName) } impl RegistryRef { fn resolve(config: &Config) -> Result<RegistryUri> { // } } struct RegistryUri(Uri);

Why do you need to remember aliases here?

Like you can do the resolving as late as possible (and still keep the original registry ref around)

mara-schulke · 2024-12-05T23:41:06Z

src/registry/mod.rs

+    /// An alias to a registry
+    Alias(String),
+    /// A resolved alias to a registry
+    ResolvedAlias {


Thus this case seems both type unsafe and unnecessary to me as you would want to go from RegistryRef to RegistryUri during alias resolution

mara-schulke · 2024-12-05T23:41:32Z

src/registry/mod.rs

+    /// Get the raw URL of the registry with any alias resolved
+    ///
+    /// # Arguments
+    /// * `config` - The configuration to use to resolve the alias


Ie this could be typesafe!

mara-schulke · 2024-12-05T23:41:50Z

src/registry/mod.rs

+    }
+
+    /// Serializer for resolved RegistryUris
+    pub fn serialize_resolved<S>(&self, serializer: S) -> Result<S::Ok, S::Error>


This aswell.

mara-schulke · 2024-12-05T23:42:17Z

src/registry/mod.rs

+///
+/// # Arguments
+/// * `url` - The URL to check
+pub fn sanity_check_url(url: &Url) -> miette::Result<()> {


Please don't make this part of the public api

mara-schulke · 2024-12-09T21:11:19Z

Okay, let's cut any refactorings or feature changes to get it merged asap. If you could tackle the code quality topics I've raised I would be glad. Once you are happy please give me another go and I'll have a look!

dgehriger and others added 30 commits June 25, 2024 21:43

Add .buffrs/config.toml registries

9446948

Add .buffrs/config.toml registry.default

b34defb

Add .buffrs/config.toml registry.default

f3ae30a

Allow dots in package names

7ad24a2

Implement config file

c34b872

Apply clippy

5a2cec3

Update CI

8843b38

Merge pull request #1 from globusmedical/feature/gm

6d23d15

Add .buffrs/config.toml registries and hierarchical packages

Fix workflow

f6d4549

Fix formatting

d7b1828

Fix formatting

b5c1e87

Fix workflow

cc83f49

Fix workflow

f99a1f6

Add buffrs install --only-dependencies

6cc69c1

Revert allowing dots in package names

5d87c44

Remove configurable proto and archive dir

017e6ac

Remove configurable proto and archive dir

b66e8d9

v0.8.1-gm3

cdec16e

Add registries

4eb1f92

Add registries

f1e88db

Add registries

2097e8b

Merge tag 'v0.9.0' into main-gm

23f3cfd

# Conflicts: # Cargo.lock # Cargo.toml # src/main.rs # src/manifest.rs # src/registry/mod.rs # tests/cmd/publish/in # tests/cmd/publish/out

Fix clippy and fmt

9fb451b

Fix vulnerabilities

7c9fe79

Fix tests

6ff055f

Fix vulnerabilities

b3427b7

Allow passing login token through API

b04395b

Enable registry alias in Proto.toml

6f1c254

Fix formatting

05e21ff

dgehriger requested a review from mara-schulke November 29, 2024 17:38

dgehriger added 4 commits November 29, 2024 18:44

Revert Cargo.lock

8d82faf

# Conflicts: # Cargo.lock

Update Cargo.lock

86624b6

Remove unused dependency

f83b816

Remove stray buf.yaml

3655fa9

dgehriger mentioned this pull request Nov 29, 2024

Feature/registry alias package globusmedical/buffrs#8

Open

dgehriger added 2 commits November 29, 2024 22:29

Fix clippy warning & test

8bf3a45

Merge branch 'feature/registry-alias' into feature/registry-alias-pac…

1d8b895

…kage # Conflicts: # Cargo.lock

mara-schulke reviewed Dec 1, 2024

View reviewed changes

dgehriger added 9 commits December 1, 2024 16:53

Update unit tests to manifest version 0.10

ab19187

Make newtype for Artifactor URIs

52fe4ef

Improve alias resolution message

24b728d

rename cert_validation_policy -> policy

a6a41b4

Revert to private

cc82f34

Use newtype for ResolvedManifest

f77d2fa

Merge branch 'feature/registry-alias-package' into feature/registry-a…

fe07689

…lias

Revert moving CLI to lib

68d5f43

Fix unit tests

7b2da4b

dgehriger added 2 commits December 1, 2024 19:02

Revert generation of proto.rs

a5651c1

Add Tarball newtype

26395f5

dgehriger requested a review from mara-schulke December 1, 2024 18:19

dgehriger added 2 commits December 2, 2024 09:20

Upgrade dependencies to address cargo deny check

689b703

Revert Cargo.lock format version to 3

d96905d

mara-schulke requested changes Dec 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/registry alias #273

Feature/registry alias #273

dgehriger commented Nov 21, 2024 •

edited

Loading

dgehriger commented Nov 29, 2024

mara-schulke commented Nov 30, 2024

mara-schulke commented Dec 1, 2024

mara-schulke commented Dec 1, 2024

dgehriger commented Dec 1, 2024 •

edited

Loading

mara-schulke left a comment

mara-schulke Dec 5, 2024

mara-schulke Dec 5, 2024

mara-schulke Dec 5, 2024

mara-schulke Dec 5, 2024

dgehriger Dec 6, 2024

mara-schulke Dec 9, 2024

mara-schulke Dec 5, 2024

dgehriger Dec 6, 2024 •

edited

Loading

mara-schulke Dec 9, 2024

mara-schulke Dec 9, 2024

mara-schulke Dec 5, 2024

mara-schulke Dec 5, 2024

mara-schulke Dec 5, 2024

mara-schulke Dec 5, 2024

mara-schulke commented Dec 9, 2024

Feature/registry alias #273

Are you sure you want to change the base?

Feature/registry alias #273

Conversation

dgehriger commented Nov 21, 2024 • edited Loading

buffrs --insecure ...

buffrs install --only-dependencies

buffrs install --generate-buf-yaml <file> [<prefix>]

Repository Configuration File (repo-root/.buffrs/config.toml)

Registry Aliases

Aliases on the Command Line

Aliases in Proto.toml

Alias resolution during package and publish

Improved Path Dependency Handling

Nested Path Dependencies

Path Dependency and Registry Dependency

Code Refactoring

CLI code moved to cli.rs

dgehriger commented Nov 29, 2024

mara-schulke commented Nov 30, 2024

mara-schulke commented Dec 1, 2024

mara-schulke commented Dec 1, 2024

dgehriger commented Dec 1, 2024 • edited Loading

mara-schulke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgehriger Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mara-schulke commented Dec 9, 2024

dgehriger commented Nov 21, 2024 •

edited

Loading

`buffrs --insecure ...`

`buffrs install --only-dependencies`

`buffrs install --generate-buf-yaml <file> [<prefix>]`

Alias resolution during `package` and `publish`

dgehriger commented Dec 1, 2024 •

edited

Loading

dgehriger Dec 6, 2024 •

edited

Loading