Skip to content

Refactor xapi-storage-script to use modules #6191

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

Conversation

Vincent-lau
Copy link
Contributor

The bind function in xapi-storage-script has lots of implementaions all over the place, making it hard to maintain. Use module abstractions to separate different storage functions.

The tricky bit of this is the need to pass version and volume_script_dir into each storage function calls, and these two variables are determined at runtime. Hence functors are used for this purpose, once the volume_script_dir is determined when bind is called, pass this as inside the RuntimeMeta module to the relevant implementations. The version global variable, however, is populated when Query.query is called, so create an alias in the RuntimeMeta module so that it can be used in the storage function implementations.

@Vincent-lau Vincent-lau marked this pull request as draft January 2, 2025 13:22
@Vincent-lau Vincent-lau force-pushed the private/shul2/storage-script-modules branch from 25fcecf to d779a25 Compare January 2, 2025 16:18
@Vincent-lau Vincent-lau self-assigned this Jan 2, 2025
@Vincent-lau Vincent-lau force-pushed the private/shul2/storage-script-modules branch from d779a25 to 35948a7 Compare January 2, 2025 16:35
@Vincent-lau Vincent-lau marked this pull request as ready for review January 3, 2025 14:57
@Vincent-lau Vincent-lau removed their assignment Jan 6, 2025
Copy link
Contributor

@changlei-li changlei-li left a comment

Choose a reason for hiding this comment

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

The original bind function is too long. I like this refactor.

@Vincent-lau Vincent-lau force-pushed the private/shul2/storage-script-modules branch from 35948a7 to e44677c Compare January 23, 2025 13:09
@Vincent-lau Vincent-lau force-pushed the private/shul2/storage-script-modules branch from e44677c to 136854e Compare January 23, 2025 13:25
@Vincent-lau Vincent-lau added this pull request to the merge queue Jan 31, 2025
@edwintorok edwintorok removed this pull request from the merge queue due to a manual request Jan 31, 2025
@edwintorok edwintorok added this pull request to the merge queue Jan 31, 2025
@Vincent-lau Vincent-lau removed this pull request from the merge queue due to a manual request Jan 31, 2025
The `bind` function in xapi-storage-script has lots of implementaions
all over the place, making it hard to maintain. Use module abstractions
to separate different storage functions.

The tricky bit of this is the need to pass `version` and
`volume_script_dir` into each storage function calls, and these two
variables are determined at runtime. Hence functors are used for this
purpose, once the `volume_script_dir` is determined when `bind` is
called, pass this as inside the `RuntimeMeta` module to the relevant
implementations. The `version` ref cell is populated when `Query.query`
is called, which should be the first call of each plugin. Once it is
populated, it will then be used by different plugin functions. We do
need a separate version for each plugin though, so create a new module
(which contains a new version) each time bind is called, just as before.

Signed-off-by: Vincent Liu <shuntian.liu2@cloud.com>
@Vincent-lau Vincent-lau force-pushed the private/shul2/storage-script-modules branch from 136854e to 0b47e27 Compare January 31, 2025 12:20
@Vincent-lau Vincent-lau added this pull request to the merge queue Jan 31, 2025
Merged via the queue into xapi-project:master with commit 9be7780 Jan 31, 2025
15 checks passed
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.

4 participants