Use StoreOpts for secret store API #1150
Conversation
|
Putting in WIP until I get the Podman half done, so this PR doesn't break Podman |
|
LGTM |
There was a problem hiding this comment.
Just a nit. Feel free to merge once the nit is addressed and you feel ready to merge from the Podman/Buildah side.
Thank you, @ashley-cui !
| @@ -102,6 +102,12 @@ type SecretsDriver interface { | |||
| Delete(id string) error | |||
| } | |||
|
|
|||
| type StoreOptions struct { | |||
There was a problem hiding this comment.
Please add comments to the type and the fields.
ashley-cui
force-pushed
the
secretsapi
branch
from
831bbbc to
4923b80
Compare
|
@containers/podman-maintainers PTAL and merge, I'm blocking vendoring ATM and this should unblock it |
|
/lgtm |
| if options.Metadata == nil { | ||
| options.Metadata = make(map[string]string) | ||
| } | ||
| if options.Labels == nil { | ||
| options.Labels = make(map[string]string) | ||
| } | ||
| if options.DriverOpts == nil { | ||
| options.DriverOpts = make(map[string]string) |
There was a problem hiding this comment.
Is there any specific reason why they cannot be nil? If these are optional I always prefer nil pointers vs empty maps.
There was a problem hiding this comment.
It's either we check here and store empty maps, or later on, when getting a secret, checking if the maps != nil each time. I feel like storing empty maps are a little less error prone, but thats also personal preference.
pkg/secrets/secrets.go
Outdated
| // DriverOptions is other metadata needed to use the driver | ||
| DriverOpts map[string]string | ||
| // Metadata stores other metadata on the secret | ||
| Metadata map[string]string |
There was a problem hiding this comment.
| // DriverOptions is other metadata needed to use the driver | |
| DriverOpts map[string]string | |
| // Metadata stores other metadata on the secret | |
| Metadata map[string]string | |
| // DriverOptions are extra options used to run this driver | |
| DriverOpts map[string]string | |
| // Metadata stores extra metadata on the secret | |
| Metadata map[string]string |
Maybe something like this?
Clean up store code by using StoreOpts instead of multiple args as opts for storing secrets. Signed-off-by: Ashley Cui <acui@redhat.com>
ashley-cui
force-pushed
the
secretsapi
branch
from
4923b80 to
f59b942
Compare
|
@containers/podman-maintainers Once again looking for a slash lgtm :') |
|
/lgtm |
|
/approve |
|
/approve |
2 similar comments
|
/approve |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashley-cui, cevich, edsantiago, giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Clean up store code by using StoreOpts instead of multiple args as opts for storing secrets.
Signed-off-by: Ashley Cui acui@redhat.com
Mentioned in: #1147 (comment)