Skip to content

Conversation

@davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Nov 20, 2024

This PR:

  • Fixes the problem described in ls-apis broken due to the new clickhouse admin APIs #7121.
  • Adds a new check that the tool found a producer for every API in the metadata. (I had to add an exception for "dsc" because it's not deployed. But the exception is driven by metadata, not hardcoded.) This ensures that we catch similar problems in CI in the future.

These are separated into two commits if you want to see which part is which. They're in the reverse order -- I added the test first to make sure it would fail without the fix.

Fixes #7121.

@davepacheco
Copy link
Collaborator Author

With just the check in place but no fix, the command fails as expected:

dap@ivanova omicron-work $ git reset --hard d7a02268836aaf58ef2fa37be1c715eb1647cf10
HEAD is now at d7a022688 add check that all APIs are produced at least once
dap@ivanova omicron-work $ cargo xtask ls-apis apis
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.53s
     Running `target/debug/xtask ls-apis apis`
   Compiling omicron-ls-apis v0.1.0 (/home/dap/omicron-work/dev-tools/ls-apis)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 4.10s
     Running `target/debug/ls-apis apis`
loading metadata for workspace omicron from current workspace
loading metadata for workspace propolis from /home/dap/.cargo/git/checkouts/propolis-12517f89d3d9f483/8f8fbb7/Cargo.toml
loading metadata for workspace maghemite from /home/dap/.cargo/git/checkouts/maghemite-de41bdd6c14939ab/056283e/Cargo.toml
loading metadata for workspace crucible from /home/dap/.cargo/git/checkouts/crucible-f3b5bdecdc6486d6/b7b9d56/Cargo.toml
loading metadata for workspace dendrite from /home/dap/.cargo/git/checkouts/dendrite-627b239a911c6241/4cdc7d7/Cargo.toml
note: ignoring Cargo dependency from crucible-pantry -> ... -> crucible-control-client
note: ignoring Cargo dependency from omicron-sled-agent -> dns-server
Error: error: found no producer for API with client package name "clickhouse-admin-keeper-client" in any deployment unit (should have been one that contains server package "clickhouse-admin-api")

Copy link
Contributor

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

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

Looks good!

/// human-readable notes about this API
pub notes: Option<String>,
/// whether this package is ever expected to be deployed in a real system
dev_only: Option<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What does None mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the comment to reflect that None means an API is deployed on real systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks. Worth using serde's default for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We definitely could. I don't have a strong feeling either way.

@davepacheco davepacheco enabled auto-merge (squash) November 21, 2024 01:11
@sunshowers sunshowers disabled auto-merge November 21, 2024 02:29
@sunshowers sunshowers enabled auto-merge (squash) November 21, 2024 02:30
@davepacheco davepacheco disabled auto-merge November 21, 2024 03:44
@davepacheco davepacheco enabled auto-merge (squash) November 21, 2024 03:45
@davepacheco davepacheco merged commit e951652 into main Nov 21, 2024
16 checks passed
@davepacheco davepacheco deleted the dap/api-dag branch November 21, 2024 05:23
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.

ls-apis broken due to the new clickhouse admin APIs

3 participants