add cargo xtask ls-apis
#6561
add cargo xtask ls-apis
#6561
Conversation
| via path+file:///home/dap/omicron-merge/out/ls-apis/checkout/maghemite/ddmd#0.1.0 | ||
| consumed by: mgd (maghemite/mgd) | ||
| via path+file:///home/dap/omicron-merge/out/ls-apis/checkout/maghemite/mg-lower#0.1.0 | ||
| via path+file:///home/dap/omicron-merge/out/ls-apis/checkout/maghemite/mgd#0.1.0 | ||
| consumed by: omicron-nexus (omicron/nexus) | ||
| via path+file:///home/dap/omicron-merge/nexus#[email protected] | ||
| consumed by: omicron-sled-agent (omicron/sled-agent) | ||
| via path+file:///home/dap/omicron-merge/sled-agent#[email protected] | ||
| consumed by: tfportd (dendrite/tfportd) | ||
| via path+file:///home/dap/omicron-merge/out/ls-apis/checkout/dendrite/tfportd#0.1.0 | ||
| consumed by: wicketd (omicron/wicketd) | ||
| via path+file:///home/dap/omicron-merge/wicketd#0.1.0 |
There was a problem hiding this comment.
Thoughts on replacing dap with something generic like user?
There was a problem hiding this comment.
I wouldn't mind but I try to avoid munging output that's presented as actual output from a command. It does seem pretty safe here, admittedly.
There was a problem hiding this comment.
I'm fine with either :)
| // TODO As of this writing, we have two distinct packages called | ||
| // "dpd-client": |
There was a problem hiding this comment.
Oh yeah this isn't good -- definitely worth fixing, maybe even as a blocker to this PR (which would make this comment moot). Probably easiest to rename the one in Omicron.
There was a problem hiding this comment.
I think this is not that easy because it may be used by dependencies that aren't in Omicron. I will definitely file an issue for this. I don't consider it a blocker because this is already a problem and not really made worse by having this tool.
There was a problem hiding this comment.
hmm, yeah that's unfortunate. Thanks for filing the issue.
This reverts commit c7a5a0b.
.github/buildomat/build-and-test.sh
Outdated
| # Do some test runs of the `ls-apis` command. | ||
| banner ls-apis | ||
| ( | ||
| source ./tools/include/force-git-over-https.sh; |
There was a problem hiding this comment.
hmm, doesn't buildomat already take care of this? @jclulow
There was a problem hiding this comment.
I think what's essential here is the export CARGO_NET_GIT_FETCH_WITH_CLI=true. Without this, the CI job fails because it can't access a token that can be used to clone the https URL.
edit: I didn't actually test only setting that variable. But the previous commit on this branch tried removing the source altogether and it failed in that way. Since this is already an https URL, I think that part of the sourced script isn't critical. But the error message for the failure even mentions that if it works from the git CLI, then you should set this variable. I opted to source this script anyway instead of just setting the variable somewhat arbitrarily but it feels like it communicates more context about what's going on.
| [[apis]] | ||
| client_package_name = "installinator-client" | ||
| label = "Wicketd Installinator" | ||
| server_package_name = "installinator-api" |
There was a problem hiding this comment.
Might be worth adding a note here saying that "installinator-api" is named after the client, unlike our usual convention of naming APIs after the server.
| known_server_packages: &'a BTreeMap<ServerPackageName, &'a ApiMetadata>, | ||
|
|
||
| // outputs (structures that we're building up) | ||
| errors: Vec<anyhow::Error>, |
There was a problem hiding this comment.
super minor: I generally like passing in error collection like this as a parameter rather than a field, but since the struct is mutable anyway it's a matter of taste. (Essentially, it indicates to callers that this is how errors are handled.)
If it were immutable then passing error collection in as a mutable param is basically required.
There was a problem hiding this comment.
Yeah, that's interesting. But I think this would be more unwieldy in this case. This is kind of like a builder with a bunch of different methods that could produce errors. I think it'd be much more annoying to have to pass errors to all of the methods.
|
I think CI failed here on the Helios job because the GitHub token expired during the job. The job took 94 minutes and the token is only good for 60. I will probably fix this by moving the new invocations to a separate job. |
The goal of this tool is to programmatically identify potential API dependencies (for Dropshot/Progenitor-based APIs) and use developer-maintained metadata to annotate these dependencies and filter out the bogus ones. While working on this I've found it has a lot of potential for scope creep. I'm creating the PR at this point because I think it's a useful stopping point for now and I'd rather the tooling be in the repo than sitting elsewhere.
The new README has a good summary of what's here.