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(gengapic): add snippets #1220

Merged
merged 91 commits into from
Mar 8, 2023
Merged

feat(gengapic): add snippets #1220

merged 91 commits into from
Mar 8, 2023

Conversation

quartzmo
Copy link
Member

@quartzmo quartzmo commented Jan 27, 2023

The snippet output added in this change is known to differ from legacy snippetgen output as follows:

  1. In new snippet metadata, resultType pointer values contain the * prefix. (In current metadata, the * does not appear.)
  2. In new snippet file license headers, https replaces current headers' usage of http.
  3. In new snippet file "generated by" header comments, the source is changed to the protoc plugin.
  4. In new snippet file imports, a blank line that currently appears before iam imports is now removed. Note that this decreases the segments.end value by 1 in the new snippet metadata.
            datacatalog "cloud.google.com/go/datacatalog/apiv1beta1"
    -
            iampb "google.golang.org/genproto/googleapis/iam/v1"
  5. In new snippet metadata, clientMethod.method values are changed to the actual protobuf source values for mixin methods:
             "method": {
               "shortName": "GetLocation",
    -          "fullName": "google.cloud.kms.v1.EkmService.GetLocation",
    +          "fullName": "google.cloud.location.Locations.GetLocation",
               "service": {
    -            "shortName": "EkmService",
    -            "fullName": "google.cloud.kms.v1.EkmService"
    +            "shortName": "Locations",
    +            "fullName": "google.cloud.location.Locations"
               }

@quartzmo quartzmo force-pushed the snippets-1 branch 4 times, most recently from eb7a80e to f266e8e Compare February 6, 2023 22:05
@quartzmo quartzmo marked this pull request as ready for review February 15, 2023 18:41
@quartzmo quartzmo requested review from a team as code owners February 15, 2023 18:41
@snippet-bot
Copy link

snippet-bot bot commented Feb 15, 2023

Here is the summary of possible violations 😱

There are 2 possible violations for not having product prefix.
There are format violations for 2 region tags.

The end of the violation section. All the stuff below is FYI purposes only.


Here is the summary of changes.

You are about to add 3 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

Sorry, something went wrong.

@quartzmo quartzmo marked this pull request as draft February 15, 2023 22:11
@quartzmo quartzmo marked this pull request as ready for review February 23, 2023 21:26
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

This is pretty cool, @quartzmo well done.

@noahdietz
Copy link
Collaborator

Can you add a new config file for SnippetBot to stop it from bothering us about region tags in generator code? Add the following to .github/snippet-bot.yml:

# The generator prints region tags and snippet-bot wants to correct
# the printing code.
ignoreFiles:
  - "internal/**"

@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 3, 2023
@quartzmo quartzmo requested review from noahdietz and codyoss March 3, 2023 17:51
@quartzmo quartzmo added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 6, 2023
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Just a couple of small things

Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

I am confused why changes that have already landed in main are showing up in the diffs here, is anyone else seeing that? Like ci.yaml and deps.yaml changing to bazel verison 6.0.0. And the CHANGELOG.md including the latest release...why are they in this PR?

@quartzmo
Copy link
Member Author

quartzmo commented Mar 6, 2023

@noahdietz I also see those diffs in the file view for this PR in GH. But I do not see them locally. The following returns no differences:

git diff main -- .github/workflows/ci.yaml

I guess maybe GH might be confused by so many commits?

@quartzmo quartzmo added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 6, 2023
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

Good call on doing the conditional refactor now, it made things a lot cleaner.

@quartzmo quartzmo merged commit 98f7a13 into googleapis:main Mar 8, 2023
@quartzmo quartzmo deleted the snippets-1 branch March 8, 2023 18:55
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

4 participants