Plugin initialization and configuration#408
Conversation
Install a specific version of golangci-lint as part of the lint Makefile target, and use that version for linting. There are often incompatible changes from version to version of golangci-lint. Some linters become deprecated, replaced by new onces. Occasionally, .golangci.yml format changes. This means we often need to deal with unrelated linter issues before we can submit pull requests. Moreover, different projects may expect different linter versions, further necessitating upgrading/downgrading of the system-installed binary. To get around this, install a repo-local binary of specific version automatically as a precondition to running the lint Makefile target. The version can then be upgrading in a controlled way, independent of other changes, without impacting other projects. Signed-off-by: Sergei Trofimov <sergei.trofimov@arm.com>
| // SetString sets the specified key to the specified string value and returns a | ||
| // pointer to the Parameters object. If the key already exists, it is | ||
| // overwritten. | ||
| func (o *Parameters) SetString(key, value string) *Parameters { |
There was a problem hiding this comment.
I am little confused, as to why you need functions on line 138, 145, 152, 159, 166???
Shouldn't the function Set(key string, val any) is enough to handle supported types?
There was a problem hiding this comment.
Set() may return an error if you give it an unsupported type. The typed SetXXX() methods cannot error, and so can return *Paremeters, allowing chaining, which allows for a more convenient construction of Parameters (e.g. see test code); or, alternatively, you can just ignore the return value since you don't have to worry about error checking.
There was a problem hiding this comment.
Not sure, providing alternative methods to accomplish the same objective is a great idea. The documentation on // Set() can be improved to state clearly of the supported types and indicating an erroneous results (if passed with un-supported types) thus setting the correct expectations.
There was a problem hiding this comment.
Not sure, providing alternative methods to accomplish the same objective is a great idea.
Why not? The typed SetXXX() methods are more convenient, and what you would typically want to use if you know the type of what you're putting into parameters since you don't have to do (superfluous) error checking. Set() is more flexible and more convenient when you don't know the type of you're putting in, since you don't have to resolve the type beforehand.
Set() can be improved to state clearly of the supported types and indicating an erroneous results (if passed with un-supported types) thus setting the correct expectations
Done.
yogeshbdeshpande
left a comment
yogeshbdeshpande
left a comment
There was a problem hiding this comment.
left some comments, will complete review by tomorrow (10/04)
setrofim
force-pushed
the
setrofim/plugin
branch
2 times, most recently
from
402da78 to
d69aafd
Compare
jraman567
left a comment
jraman567
left a comment
There was a problem hiding this comment.
Thanks for the PR, @setrofim ! It looks good to me.
Just to summarize, the PR addresses two of the issues we were discussing about scheme initialization/loading.
- If scheme's Init fails, this PR skips that scheme without crashing the process
- It also supplies scheme-specific parameters
I understand @yogeshbdeshpande is still reviewing, so I'd wait for his approval also. Thank you!
Add an Init() method to IPluggable. This gets invoked immediately after a plugin is first loaded, giving opportunity for early one-time initialization. This method takes a Parameters object that contain configuration for the plugin. SchemeImplementationWrapper has been updated to provide a default implementation, so implementing Init() is optional for attestation schemes. Note: as of this commit, the feature is currently unused as none of the existing attestation schemes require initialization or configuration. Signed-off-by: Sergei Trofimov <sergei.trofimov@arm.com>
setrofim
force-pushed
the
setrofim/plugin
branch
from
d69aafd to
f0eb860
Compare
thomas-fossati
left a comment
thomas-fossati
left a comment
There was a problem hiding this comment.
Both commits LGTM, thanks!
4 participants
Add an
Init()method toIPluggable. This gets invoked immediately after a plugin is first loaded, giving opportunity for early one-time initialization. This method takes aParametersobject that contain configuration for the plugin.SchemeImplementationWrapperhas been updated to provide a default implementation, so implementing Init() is optional for attestation schemes.Also, update the
lintMakefile target to download and use a local version ofgolangci-lintto avoid possible compatibility issues with the system-installed version.