Closed jgawor closed 2 months ago
Thanks for the PR! I'll try to review within the next week or so. Initial thoughts:
Being able to use the group name instead of having to use the ID is a great addition - that way it's consistent with how secret interpolation works, where using the secret name can be used instead of the ID
Being able to interpolate fields of the secret without using jsonPath
is also nice. It might be a bit confusing that the IBM Cloud SM backend has 2 different path syntax options but I guess as long as the docs are clear it's OK.
On that note, please update the docs to describe the changes made here
Again, thanks for the contribution - code, tests and all. Glad to see this could be implemented without breaking existing behavior as well!
Attention: Patch coverage is 72.82609%
with 25 lines
in your changes are missing coverage. Please review.
Project coverage is 72.00%. Comparing base (
42a43f0
) to head (5832898
).
Files | Patch % | Lines |
---|---|---|
pkg/backends/ibmsecretsmanager.go | 72.82% | 18 Missing and 7 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@jkayani I updated the docs. Please take a look.
@jkayani I addressed your comments and updated the PR. Thanks! As to the motivation for the PR, it was the overall ease of use. For group names, that's simple. Using the group ids is non-portable from one instance of IBM Secrets Manager to another (e.g. in different regions). Also, trying to decipher the security group id from a path (and mapping it to a group in IBM Secrets Manager) is much harder than a name. As to the simplified path syntax, we actually use the vault plugin with two backends: IBM and AWS. Injecting secrets with the AWS Secrets Manager is simple and does not usually jsonPath
or other modifiers. With the existing IBM backend, the jsonPath
modifier is pretty much always needed. So, the idea was to remove that requirement and maybe make moving between the backends a bit simpler.
Description
<path:ibmcloud/arbitrary/secrets/groups/123>
one can use<path:ibmcloud/arbitrary/secrets/groups/myGroupName>
jsonPath
modifier. For example, instead of<path:ibmcloud/imported_cert/secrets/groups/123#my-cert-secret#previous | jsonPath {.certificate}>
one can use<path:ibmcloud/imported_cert/secrets/groups/123/my-cert-secret/certificate#previous>
The existing behavior is unchanged.
Checklist
Please make sure that your PR fulfills the following requirements:
go mod tidy -compat=1.21
to ensure only the minimum is pulled in.Type of Change
Other information