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

Update resources proto with GetSecretNames and CreateSecret. #3914

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

absoludity
Copy link
Contributor

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

Adds the proto messages and rpc's for both GetSecretNames and CreateSecret.

The current dashboard code using shared/Secret is just the add/update AppRepository form. It currently pulls the actual secret to re-use in the form, which I'm going to change when updating the dashboard, so that we don't need to pull the actual secret data. This allows the resources API to restrict itself to getting the secret names (with types), and creating a secret. The user will still be able to update a value, but not by editing the existing value (which is in a password input type anyway).

Implementation of these methods to follow.

Benefits

Possible drawbacks

Applicable issues

Additional information

Comment on lines +3797 to +3807
"SECRET_TYPE_OPAQUE_UNSPECIFIED",
"SECRET_TYPE_SERVICE_ACCOUNT_TOKEN",
"SECRET_TYPE_DOCKER_CONFIG",
"SECRET_TYPE_DOCKER_CONFIG_JSON",
"SECRET_TYPE_BASIC_AUTH",
"SECRET_TYPE_SSH_AUTH",
"SECRET_TYPE_TLS",
"SECRET_TYPE_BOOTSTRAP_TOKEN"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding SECRET_TYPE_ as a prefix here produces code with redundant names like SecretType_SECRET_TYPE_OPAQUE_UNSPECIFIED (in cmd/kubeapps-apis/gen/plugins/resources/v1alpha1/resources.pb.gobelow).
Not a big problem at all, mainly readability. Couldn't we avoid it? Maybe it is a requirement for the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know it feels it (and I did originally do it without the prefix), but then buf lint fails as this is one of their lint rules. They explain why here: https://docs.buf.build/lint/rules#enum_value_prefix

Base automatically changed from 3896-namespaces-via-plugin-3 to master December 13, 2021 18:53
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Great, thanks!

// Context
//
// The context for which the secret names are being fetched.
kubeappsapis.core.packages.v1alpha1.Context context = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I used the same for the SA method... but I'm wondering now whether we should keep a resources-plugin-specific Context message to decouple it from the packages API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The resources plugin is coupled because of it's main purpose: to get the resources for an installed package. So even if we add a separate context, the module is still coupled by design for that one method?

@absoludity absoludity merged commit ece5c3f into master Dec 13, 2021
@absoludity absoludity deleted the 3896-secrets-backend branch December 13, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants