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

feat(snippetgen): generate mock input for required fields #941

Merged
merged 17 commits into from
Aug 18, 2021

Conversation

busunkim96
Copy link
Contributor

@busunkim96 busunkim96 commented Jun 29, 2021

Generate mock field inputs for all required fields.

  • For oneofs, the first option is selected.
  • For enums, the last option is selected (as 0 is often unspecified).
  • Mock input for string fields that reference resources (resource_pb2.resource_reference) is the pattern provided in the protos.

TODOs:

  • When a resource's pattern is * this results in unhelpful strings

    message Asset {
      option (google.api.resource) = {
        type: "cloudasset.googleapis.com/Asset"
        pattern: "*"
      };
    request = asset_v1.BatchGetAssetsHistoryRequest(
       parent="*",
    )
  • Map fields are not yet handled.

Other changes:

  • Fields are set like foo.bar rather than through dict access foo["bar"]. This is because the former allows you to set fields nested more than one field deep foo.bar.baz.

    Before:

    output_config = {}
    output_config["gcs_destination"]["uri"] = "uri_value" # fails as there is no nested dict

    After:

    output_config = asset_v1.IamPolicyAnalysisOutputConfig()
    output_config.gcs_destination.uri = "uri_value"

@snippet-bot
Copy link

snippet-bot bot commented Jun 29, 2021

Here is the summary of changes.

You are about to add 2 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2021
@busunkim96 busunkim96 marked this pull request as ready for review June 29, 2021 21:38
@busunkim96 busunkim96 requested a review from a team as a code owner June 29, 2021 21:38
Copy link

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

Okay @busunkim96 - I looked at some - didn't make it through all, but a lot of my comments will apply to multiple samples, I think. Hope it was at least a little helpful!

@@ -35,6 +35,7 @@ def sample_batch_get_assets_history():

# Initialize request argument(s)
request = asset_v1.BatchGetAssetsHistoryRequest(
parent="*",

Choose a reason for hiding this comment

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

I'm not familiar with this syntax - is this asterisk something that's typical for this service?

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 * wouldn't actually work if passed to the service. The snippetgen logic pulls the first resource pattern out of the protos, and in some uncommon cases it is just a * rather than some more specific pattern like projects/{project}. The * pattern creates problems for generating resource helper methods and tests too - see #701

@busunkim96
Copy link
Contributor Author

@software-dov Thank you for the review! PTAL

Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

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

Mostly nitpicking and one minor concern, looks great! Thanks for doing this, and I hope the existing samplegen code wasn't too unpleasant to work with.

gapic/samplegen/samplegen.py Show resolved Hide resolved
gapic/schema/wrappers.py Outdated Show resolved Hide resolved
gapic/schema/wrappers.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants