Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move lxd/response to shared/response #13255

Closed
markylaing opened this issue Apr 3, 2024 · 1 comment
Closed

Move lxd/response to shared/response #13255

markylaing opened this issue Apr 3, 2024 · 1 comment
Assignees
Labels
Improvement Improve to current situation
Milestone

Comments

@markylaing
Copy link
Contributor

markylaing commented Apr 3, 2024

Many downstream packages are using the lxd/response package. #13252 removed the import of lxd/db from lxd/response so that downstream package can avoid additional imports, but if the package is being used externally it should be moved under shared. This is a little tricky because lxd/response also imports lxd/ucred and lxd/util. lxd/ucred imports lxd/endpoints/listeners and so on. So we'll need to evaluate which utils are specific to LXD and which could be used externally.

Additionally, we can think about moving some nice utils from LXD cloud into the shared package. E.g. the request parser in LXD Cloud provides a clean way to extract request info: https://github.com/canonical/lxd-cloud/blob/main/internal/rest/request/parser.go

After this is done, we should evaluate the imports of e.g. microcluster and microcloud to ensure they are only importing from shared or client.

@markylaing markylaing self-assigned this Apr 3, 2024
@tomponline tomponline added this to the lxd-6.1 milestone Apr 4, 2024
@tomponline tomponline added the Improvement Improve to current situation label Apr 4, 2024
@tomponline tomponline modified the milestones: lxd-6.1, lxd-6.2 May 12, 2024
@markylaing
Copy link
Contributor Author

We have experienced issues with downstream packages using lxd/response. See #14408

In that PR, we decided not to move the package as it would require all downstreams to update their import path. I'm going to close this for now as we will be evaluating all libraries used by downstreams separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Improve to current situation
Projects
None yet
Development

No branches or pull requests

2 participants