Skip to content
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

Add labels to secrets #1147

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Conversation

ashley-cui
Copy link
Member

Allow secrets to be labeled. Add new field in secrets package called label.

Signed-off-by: Ashley Cui acui@redhat.com

Part of: containers/podman#14917

Allow secrets to be labeled. Add new field in secrets package called
label.

Signed-off-by: Ashley Cui <acui@redhat.com>
@ashley-cui
Copy link
Member Author

I feel like labels are important enough and different enough from metadata (especially since each label is a key-value pair, and we're using metadata for stuff like kube) that they deserved a special field in the secrets package. @vrothberg let me know what you think

@rhatdan
Copy link
Member

rhatdan commented Sep 7, 2022

LGTM

@@ -129,7 +131,7 @@ func NewManager(rootPath string) (*SecretsManager, error) {
// Store takes a name, creates a secret and stores the secret metadata and the secret payload.
// It returns a generated ID that is associated with the secret.
// The max size for secret data is 512kB.
func (s *SecretsManager) Store(name string, data []byte, driverType string, driverOpts map[string]string, metadata map[string]string) (string, error) {
func (s *SecretsManager) Store(name string, data []byte, driverType string, driverOpts map[string]string, metadata map[string]string, labels map[string]string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some on-going conversations about stabilizing containers/common and bump it to v1.0.0. That means, we need to avoid breaking the API by all means.

Since this is already a breaking change, I want to take a step back and check what we can do. I think we should follow the type *Options struct pattern.

func (s *SecretsManager) Store(name string, data []byte, driverType string, options StoreOptions) (string, error) {
   // ...
}

It is probably worth taking a look at the other APIs as well. Feel free to merge this PR as is and do the API work in a separate PR (or commit).

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/hold

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 8, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, vrothberg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Sep 8, 2022
@vrothberg
Copy link
Member

I feel like labels are important enough and different enough from metadata (especially since each label is a key-value pair, and we're using metadata for stuff like kube) that they deserved a special field in the secrets package. @vrothberg let me know what you think

I do not feel strongly about either option. Both look reasonable to me.

@ashley-cui
Copy link
Member Author

/hold cancel

@Luap99
Copy link
Member

Luap99 commented Sep 9, 2022

If merge stuff with breaking changes we should ensure that there is a podman/buildah PR with the required fixes ready to go. Now this just blocks us from vendoring new c/common versions. So I am unable to get my my PR into Podman #1151

@ashley-cui
Copy link
Member Author

@Luap99 My bad, I'm okay with reverting this commit if you need common in ASAP, as I'm changing this again in #1150 anyway.

@Luap99
Copy link
Member

Luap99 commented Sep 9, 2022

No, my stuff can wait until you make your podman changes. I just want to make sure we stop doing this unless we have stuff ready in podman so we do not have to block new vendor dances.

@ashley-cui
Copy link
Member Author

ashley-cui commented Sep 9, 2022

@Luap99 FYI, containers/podman#15723 and #1150 should unblock you, sorry for the mess

@Luap99
Copy link
Member

Luap99 commented Sep 9, 2022

Thanks

@ashley-cui ashley-cui deleted the label branch October 4, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants