-
Notifications
You must be signed in to change notification settings - Fork 41
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
Create a generic Interpolation plugin #732
Conversation
… into interpolation-plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @manushak
thanks for working on this but it doesn't meet the issue spec. The plugin should be generic, by which I mean it should interpolate any arbitrary data that is passed to it in the way described in the acceptance criteria in the ticket, and not require specific parameters such as vcpus-allocated
. Please check the ticket description to see how the plugin should work.
@jmcook1186 I've updated, please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed some fixes - now satisfied this works as expected. Added some new validations and updated some error messages for clarity, and added some tests.
over to @narekhovhannisyan for second review |
examples/manifests/interpolation.yml
Outdated
- timestamp: 2023-07-06T00:00 | ||
duration: 3600 | ||
cpu/utilization: 45 | ||
# vcpus-allocated: 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should they stay commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no they should get deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for noticing, fixed and the manifest moved to the manifests/plugins folder
@@ -83,21 +83,22 @@ export const manifestSchema = z.object({ | |||
* Validates given `manifest` object to match pattern. | |||
*/ | |||
export const validateManifest = (manifest: any) => | |||
validate(manifestSchema, manifest, ManifestValidationError); | |||
validate(manifestSchema, manifest, undefined, ManifestValidationError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validate
function is used by the validateManifest
function, which is called from plugins too. To avoid creating a new function, I assigned index
to undefined
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case either make it last argument, or pass all the info to validate function as a single object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both options are not achievable because a plugin calls this version of the validate function: validate<z.infer>(schema, input, index). If we change it to a single object, all plugins will need to be updated accordingly. If we change the order of the arguments, we would need to provide an Error class in every plugin. I hope I have described the situation :)
Signed-off-by: Joseph Cook <[email protected]>
Signed-off-by: Joseph Cook <[email protected]>
Types of changes
A description of the changes proposed in the Pull Request