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

Secret Manager doesn't tell me which parameters are required #689

Open
JustinBeckwith opened this issue Sep 2, 2020 · 2 comments
Open
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@JustinBeckwith
Copy link
Contributor

Looking at this ol favorite:

const sm = new SecretManagerServiceClient();
const [secrets] = await sm.listSecrets({
  parent: 'projects/el-gato',
});

Looking at the listSecrets method, I see it takes an options bag. Great! I dig deeper:

listSecrets(request: protos.google.cloud.secretmanager.v1.IListSecretsRequest, options?: gax.CallOptions): Promise<[protos.google.cloud.secretmanager.v1.ISecret[], protos.google.cloud.secretmanager.v1.IListSecretsRequest | null, protos.google.cloud.secretmanager.v1.IListSecretsResponse]>;

Ok, fine, what does IListSecretsRequest look like:

/** Properties of a ListSecretsRequest. */
interface IListSecretsRequest {

    /** ListSecretsRequest parent */
    parent?: (string|null);

    /** ListSecretsRequest pageSize */
    pageSize?: (number|null);

    /** ListSecretsRequest pageToken */
    pageToken?: (string|null);
}

This has 2 separate problems:

  1. I have no idea which of these parameters are required. I know that when I ran my code without it, I got an exception for not passing a parent :)

  2. I have no idea what any of these properties mean (I mean I do, but a mortal wouldn't). Aren't there better descriptions somewhere for these properties in the protos?

@JustinBeckwith JustinBeckwith added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Sep 2, 2020
@alexander-fenster
Copy link
Contributor

This one is a valid bugreport / feature request. We do have comments in protos, but apparently protobuf.js ignores the //-style comments (it wants /* */-style comments). I even proposed the hacky way to fix it #150 but we decided it must be fixed in protobuf.js.

I think there might be an option for that, I'll try to figure out.

@alexander-fenster
Copy link
Contributor

re: required fields - we mark required fields in protos (with annotations) but those annotations don't map well into the types generated by pbts. I need to think how to make it better.

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Dec 2, 2020
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 1, 2021
@alexander-fenster alexander-fenster added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. 🚨 This issue needs some love. labels Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

3 participants