-
Notifications
You must be signed in to change notification settings - Fork 475
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
Implement the Validate RPC on built-in plugins #5303
Conversation
Something like this might help establish a pattern and reduce boilerplate: https://gist.github.com/azdagron/b55f1e70e1855cfa1aa73cb8f0027699 |
The pattern of coupling the configuration usability control and the note as an error or informational message under one API was adopted and applied to the plugins that have been reworked under this PR. |
88c039f
to
ab60481
Compare
d1bc067
to
5709867
Compare
Hi @edwbuck, I spent some time looking at the aws_kms test failures. Here is a patch that essentially removes those changes and makes |
Thank you Agustin. I read over the patch, and in all other files I didn't notice that this combined two different testing patterns, such that it configured each plugin twice, creating the conditions that eventually triggered my race condition with my additional call to plugintest.CoreConfig(...). Expect a push soon. |
The old API performed all configuration checks coupled with plugin reconfiguration under the Configure() func. The new API adds a Validation() func that only performs configuration checks but has no impact on the running plugin. To facilitate easier user, the pluginconf package was added that makes it easier to handle the merged code streams through a pluginconf.Status struct that will capture the first error (for integration with Configure() while permitting the Validation() to capture all errors that can be captured. Unit tests had to be reworked, as a side-effect of using the new pluginconf package is that all plugins now automatically check their trustdomain, instead of each plugin checking it in a haphazard manner. Occasionally, very small fixes were performed on plugins, and plugin coding standards were tweaked in small ways to be more similar to each other. Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
For the "extra keys" the order of key retreival is not stable, so we must combine extra keys into the expected output. Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Ready for review two weeks ago. Please prioritize so I can stop keeping this up-to-date with the moving main branch. |
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 putting in this work, @edwbuck. One overarching comment is that a large majority of the *_test.go
changes could be reverted if we change the plugintest package to default to a valid CoreConfig (e.g. one with "example.org" provided as the trust domain). Tests that use plugintest.WithCoreConfig
would still be able to override the default.
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Update all plugins to have a Validate gRPC and centralize the existing validation logic such that it is reached from Validate and Configure gRPC calls.