Skip to content

Conversation

davepacheco
Copy link
Collaborator

Fixes #7847.

This isn't a great situation but it was already a problem before and I've extended the existing workaround for this problem.

I'm not sure what the best longer-term answer is here. Maybe we should treat apparently-distinct client packages having the same name as equivalent? Or maybe we should put the dpd OpenAPI document and its client package into its own public repo so that we don't generate many distinct Rust packages for the client?

@davepacheco davepacheco requested a review from hawkw March 21, 2025 17:46
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

I agree that the temporary workaround is unfortunate, but I also agree that it's better than having it be broken...

}
if found_in_workspaces[1].0.name() == "omicron" {
return Ok(found_in_workspaces[1]);
if server_pkgname == "dpd-client" && found_in_workspaces.len() == 3 {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we really should keep the length check here, or just remove it --- do we want to have to update this again when the fourth dpd-client shows up?1 Since we are always deciding to just return the dependency in this workspace, should we just do that unconditionally regardless of the number of workspaces? Or, do you think that having to bump up the 3 to 4 is a useful enough signal that we want this to break?

Footnotes

  1. Not that i think this is apt to happen imminently, but, like...it kind of feels like by the time something has happened three times, you gotta be prepared for it to happen any number of times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I think it makes sense to keep it. I'm glad I looked at this one and determined that yes, this is a new package called this and not some other strangeness that we didn't anticipate.

@davepacheco davepacheco enabled auto-merge (squash) March 21, 2025 18:51
@davepacheco davepacheco merged commit 132f5b7 into main Mar 21, 2025
16 checks passed
@davepacheco davepacheco deleted the dap/too-many-dpd-clients branch March 21, 2025 19:59
@davepacheco
Copy link
Collaborator Author

The underlying problem here was resolved by #7907! 🎉

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.

xtask ls-apis adoc can't handle server packages in multiple workspaces
2 participants