-
Notifications
You must be signed in to change notification settings - Fork 192
Adding 'pluginRuntimeVersion' to PluginDescriptor #3555
Conversation
Signed-off-by: Prem Kumar Kalle <pkalle@vmware.com>
Codecov Report
@@ Coverage Diff @@
## main #3555 +/- ##
==========================================
- Coverage 45.28% 44.89% -0.40%
==========================================
Files 370 416 +46
Lines 39306 39993 +687
==========================================
+ Hits 17801 17956 +155
- Misses 19894 20402 +508
- Partials 1611 1635 +24
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
nit: Update filename from |
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 @prkalle. That solution works nicely.
Since I wasn't familiar with this approach and before seeing your new PR, I was trying to think of how we could figure out the version of the runtime lib that what used when a plugin was compiled. So I started wondering if Go could tell us. I found the ReadBuildInfo()
function of the runtime/debug
Go package: https://stackoverflow.com/a/59276408
I think using this package would allow us to avoid any special build steps and simply read the dependency that was used by the plugin directly in the info
command.
I think another nice advantage is if a plugin uses a specific commit, or the main branch directly, we would get the specific commit in the version, instead of a generic "vx.y.x-dev"
If we prefer the current solution:
- What will be the workflow to set a new version of the cli-runtime lib?
- It may require changing
.github/workflows/dev_tag_retrieval_pr.yaml
for the automatic PR to include modifying the newzz_bundled_plugin_runtime_version.go
file. - Maybe also
hack/verify-dirty.sh
although I am not sure.
@@ -156,6 +156,9 @@ type PluginDescriptor struct { | |||
|
|||
// PostInstallHook is function to be run post install of a plugin. | |||
PostInstallHook Hook `json:"-" yaml:"-"` | |||
|
|||
// PluginRuntimeVersion of the plugin. Must be a valid semantic version https://semver.org/ | |||
PluginRuntimeVersion string `json:"pluginRuntimeVersion" yaml:"pluginRuntimeVersion"` |
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.
Is it useful to have this field in the PluginDescriptor?
I'm still getting familiar with the use of the PluginDescriptor, but the way I see it at the moment, is that it is used by a plugin to specify its "properties". This makes me hesitant to add a field that the plugin should not set itself.
In this PR, I gather that the reason we have this new field is that it will get printed by the info
command. Maybe we can instead change the info
command to print it? We may need to manipulate the json though.
But maybe I shouldn't worry about adding fields in the PluginDescriptor?
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 reason for adding this field in the PluginDescriptor is beacuse tanzu cli can get the properties of a plugin while invoking the plugin and can impose the compatibility conditions (ex: cli vX.Y.Z doesn't support the plugins created with plugin runtime version vA.B.C. )
Signed-off-by: Prem Kumar Kalle <pkalle@vmware.com>
Thank you @marckhouzam for the information(I was not aware of it before). It is good solution and I think we can use this approach once the cli/runtime moves to different repository because, as we are using a since we are going with the current approach , I did fixed |
// All the variables are set during build time | ||
// Note: Please !!DO NOT!!! change the name or remove this variable | ||
var ( | ||
Version string = "v0.27.0-dev" |
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.
this should now be "v0.28.0-dev" I think
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.
Is there a way to catch this thing as part of CI? I see we are ignoring this file as part of verify-dirty.sh
, if we do not skip this file, will it create any issue?
Closing this as requested changes are done as part of another PR: #3641 |
Signed-off-by: Prem Kumar Kalle pkalle@vmware.com
What this PR does / why we need it
This PR adds "PluginRuntimeVersion" to PluginDescriptor. This version would be used by tanzu core CLI to add version validations.
Which issue(s) this PR fixes
Fixes #3547
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer