Infrastructure to support backward-compatibility check #257

lquerel · 2024-07-23T23:15:22Z

Description

This PR implements the infrastructure to support backward-compatibility testing and, more generally, policies applied to multi-version registries.

The following command checks the current registry (main branch in the official repo) with a baseline registry (here, the archive v1.26.0) against a package of rules (policies) defined in the file compatibility_check.rego:

weaver registry check \
--registry https://github.com/open-telemetry/semantic-conventions.git[model] \
--baseline-registry https://github.com/open-telemetry/semantic-conventions/archive/refs/tags/v1.26.0.zip[model] \
--policy compatibility_check.rego

The following screenshot is the output of this command if we artificially remove the file registry/deprecated/container.yaml from the main branch.

Changes

Extend the registry path format to support local and remote archives in addition to local folders and git repositories. The format will also support the definition of sub-folders. Motivation: with this PR, we now have two types of registries. Without a registry path format, we would need to add many parameters to support all combinations (folder, git-url, git-archive, folder, tag).
- <local-path> (e.g. $HOME/otel-registry)
- <git-url> (e.g. https://github.com/open-telemetry/semantic-conventions.git)
- <git-url>[<sub-folder>] (e.g. https://github.com/open-telemetry/semantic-conventions.git[model])
- <git-archive-url> (e.g. https://github.com/open-telemetry/semantic-conventions/archive/refs/tags/v1.26.0.zip)
- <git-archive-url>[<sub-folder>] (e.g. https://github.com/open-telemetry/semantic-conventions/archive/refs/tags/v1.26.0.zip[model])
- Future extension: <git-url>@<tag>
- Future extension: <git-url>@<tag>[<sub-folder>]
Unify RegistryPath (merge the two existing implementations defined in the weaver and weaver_semconv crates).
Document CLI usage for the new registry path format.
Add --baseline-registry in addition to --registry (both will support the same syntax for the registry path). The --registry-git-sub-dir will be marked as deprecated.
Implement support for local and remove git archives (supported formats: .tar.gz, .zip).
Replace Weaver Cache with the more general concept of RegistryRepo supporting all the variants of RegistryPath.
Integrate --baseline-registry with the policy engine. In particular, we must decide how to expose the two registries in Rego.
Create standard SemConv Rego rules and function to ease the creation of semconv policies.
Create examples of policies to detect backward-compatibility issues.

…iding support.

…chical-config

# Conflicts: # crates/weaver_forge/src/config.rs # crates/weaver_forge/src/extensions/case.rs # crates/weaver_forge/src/lib.rs

src/registry/check.rs

jsuereth

Overall LGTM. A couple nits - Predominately I think next time the Git-Url / path reference code should be a separate PR form the policy engine changing code.

THink you can close a few open issues for this one :)

jsuereth · 2024-07-29T20:12:24Z

crates/weaver_cache/src/lib.rs

+                Self::create_parent_dirs(&valid_entry_path, archive_filename)?;
+                // Unpack returns an Unpacked type containing the file descriptor to the
+                // unpacked file. The file descriptor is ignored as we don't have any use for it.
+                _ = entry


Nit for rust - Maybe we should open a FR - that #[must_use] for Result where you ? away a () doesn't cause a warning, and then you can map_error(..) and something like .ignore_result()?

I'm not 100% sure, but I think a templated #[must_use] function returning a Result<T, Error> with T=() doesn't emit a warning.

I like the idea of the .ignore_result()? if we can specify a parameter &str representing the justification.

crates/weaver_cache/src/lib.rs

crates/weaver_cache/src/registry_path.rs

jsuereth · 2024-07-29T20:20:02Z

crates/weaver_cache/src/registry_path.rs

+    GitRepo {
+        /// URL of the Git repository
+        url: String,
+        /// Tag of the Git repository (NOT YET SUPPORTED)


This should be tag, branch or commit

Renamed refspec and updated the comment.

crates/weaver_cache/src/registry_path.rs

jsuereth · 2024-07-29T20:32:36Z

src/util.rs

+    // Add the standard semconv policies
+    // Note: `add_policy` the package name, we ignore it here as we don't need it
+    _ = engine
+        .add_policy("defaults/rego/semconv.rego", SEMCONV_REGO)


Should this only be added for a particular phase of the engine? Aren't these symbols meaningless in before_resolution or are they meant to be applied there too?

IMO, some of the rules/functions definitions could be used in any phases.

jsuereth · 2024-07-29T20:35:48Z

src/util.rs

@@ -119,9 +102,19 @@ pub(crate) fn check_policy_stage<T: Serialize>(
    policy_stage: PolicyStage,
    policy_file: &str,
    input: &T,
+    data: &[T],


Nit: If you want data to actually be generic, you need to use a different type. Right now input AND the array of data must be the same type.

At a minimum, you should have:

fn check_policy_stage<T: Serialize, U:Serialize>( policy_engine: &mut Engine, policy_stage: PolicyStage, policy_file: &str, input: &T, data: &[U],

However, if you want data to not be monomorphic, you'd need to use an HList (likely k-list) or tuple-type structure to preserve the original types.

I don't think this is what you want at all. For now, at least, I'd use a different generic for data. In the future, you may want to use a different type to encapsulate serialization more generically.

Nice catch. I made the changes.

lquerel · 2024-07-29T21:24:23Z

Overall LGTM. A couple nits - Predominately I think next time the Git-Url / path reference code should be a separate PR form the policy engine changing code.

THink you can close a few open issues for this one :)

Yes, creating two PRs was part of the initial intentions, but it got lost in the heat of the moment.

lquerel and others added 30 commits July 5, 2024 21:51

feat(forge): Support hierarchical weaver config

cb9cd58

feat(forge): Test override_with

b8faa77

feat(forge): Implement better UX for templates and config.

da7b6c7

chore(forge): Fix a unit test.

b6bb5ef

feat(forge): Support multiple paths to init a WeaverConfig with overr…

e818435

…iding support.

feat(cli): Reintroduce target concept

fdd51cc

feat(cli): Introduce --config parameter in weaver registry generate

ef59887

Merge branch 'main' into hierarchical-config

c55f85b

doc(forge): Document the organization and resolution of config files

f9bf773

doc(forge): Document the organization and resolution of config files

d42a61a

Merge branch 'main' into hierarchical-config

ec39d09

feat(forge): Improve WeaverConfig and TemplateEngine public APIs

add9388

Merge remote-tracking branch 'origin/hierarchical-config' into hierar…

0f44a4c

…chical-config

feat(forge): Remove FileContent from the public API

582516b

feat(forge): Create constants for Jinja syntax delimiters

78d4f40

Merge branch 'lquerel-hierarchical-config'

399030b

chore(forge): Remove unused filters function_name, struct_name, ...

0f2d7c7

chore(build): Merge with main and bump weaver version

d93a9c8

chore(build): Merge with main and bump weaver version

1e54041

Merge branch 'main' of https://github.com/lquerel/otel-weaver

35a405b

Merge remote-tracking branch 'upstream/main'

e5504cf

# Conflicts: # crates/weaver_forge/src/config.rs # crates/weaver_forge/src/extensions/case.rs # crates/weaver_forge/src/lib.rs

Merge branch 'open-telemetry:main' into main

0749edd

Merge branch 'open-telemetry:main' into main

49197f6

Merge branch 'open-telemetry:main' into main

35e8f75

Merge branch 'open-telemetry:main' into main

e9cabd9

feat(registry): Unify RegistryPath

1b39242

feat(CLI): Add --baseline-registry in weaver registry check command

0ed6b11

feat(cache): Add partial impl of SemConvRegistryRepo

a0de53a

feat(cache): Implement and test SemConvRegistryRepo::LocalArchive

52cae7f

feat(cache): Implement and test SemConvRegistryRepo::RemoteArchive

91900f1

lquerel added 3 commits July 25, 2024 08:16

chore(dep): Optimize dependencies to the opentelemetry crate

5ff29f3

feat(registry): Replace weaver Cache by RegistryRepo

cce4e0f

feat(registry): Load baseline registry in the check command

f8f69ff

github-advanced-security bot found potential problems Jul 25, 2024

View reviewed changes

src/registry/check.rs Fixed Show fixed Hide fixed

src/registry/check.rs Fixed Show fixed Hide fixed

lquerel added 2 commits July 26, 2024 15:38

feat(rego): Add defaults/rego/semconv.rego rules and functions

63f9a8b

feat(checker): Add support for policies on multiple registries

27e8cc2

lquerel changed the title ~~[WIP] Infrastructure to support backward-compatibility check~~ Infrastructure to support backward-compatibility check Jul 27, 2024

lquerel marked this pull request as ready for review July 27, 2024 00:02

lquerel requested a review from jsuereth as a code owner July 27, 2024 00:02

lquerel added 4 commits July 26, 2024 17:18

chore(build): Fix typos

c157e66

chore(build): Fix typos

72a79fa

chore(log): Improve logging

e89e27d

chore(build): Fix rustfmt and docs linter issues

0f346b9

jsuereth approved these changes Jul 29, 2024

View reviewed changes

chore(pr): Improve code based on Josh's feedback

59cb518

lquerel merged commit 0f5fd2f into open-telemetry:main Jul 29, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infrastructure to support backward-compatibility check #257

Infrastructure to support backward-compatibility check #257

lquerel commented Jul 23, 2024 •

edited

Loading

jsuereth left a comment

jsuereth Jul 29, 2024

lquerel Jul 29, 2024

jsuereth Jul 29, 2024

lquerel Jul 29, 2024

jsuereth Jul 29, 2024

lquerel Jul 29, 2024

jsuereth Jul 29, 2024

lquerel Jul 29, 2024

lquerel commented Jul 29, 2024

Infrastructure to support backward-compatibility check #257

Infrastructure to support backward-compatibility check #257

Conversation

lquerel commented Jul 23, 2024 • edited Loading

Description

Changes

jsuereth 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

Choose a reason for hiding this comment

lquerel commented Jul 29, 2024

lquerel commented Jul 23, 2024 •

edited

Loading