-
Notifications
You must be signed in to change notification settings - Fork 32
chore: clean up repository onboarding documentation #2788
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
base: main
Are you sure you want to change the base?
Conversation
Post processing files should remove mention of owlbot (in name) Signed-off-by: ldetmer <[email protected]>
Summary of ChangesHello @ldetmer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the documentation for library onboarding, specifically focusing on the guidelines for post-processing files. It clarifies the expected naming convention for these files and removes an outdated reference, ensuring the migration guide remains current and accurate for users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the repository onboarding guide to clarify the naming requirements for post-processing files, removing the mention of 'OwlBot'. My review includes a suggestion to further clarify the new naming convention to avoid ambiguity and re-introduces a removed suggestion about creating issues for improving post-processing logic, as it's a valuable practice.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2788 +/- ##
==========================================
- Coverage 86.74% 86.51% -0.23%
==========================================
Files 119 120 +1
Lines 10183 10336 +153
==========================================
+ Hits 8833 8942 +109
- Misses 949 987 +38
- Partials 401 407 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: ldetmer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments. Feel free to address them in a follow-up PR.
doc/repository-library-onboarding.md
Outdated
| This guide should be followed when onboarding new repositories/libraries. | ||
|
|
||
| ## Repository Setup: | ||
| 1) [Create ticket](http://go/onboard-repository-to-librarian) to onboard repository to Librarian automation. At a minimum, you should onboard to Tag and Release automation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many tickets is the person onboarding expected to file? Is it one ticket per library, one ticket per repository, or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added specification its per repository
doc/repository-library-onboarding.md
Outdated
| This guide should be followed when onboarding new repositories/libraries. | ||
|
|
||
| ## Repository Setup: | ||
| 1) [Create ticket](http://go/onboard-repository-to-librarian) to onboard repository to Librarian automation. At a minimum, you should onboard to Tag and Release automation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that this guide is the starting point for someone who doesn't know much about Librarian. In that case, Tag and Release automation might be unfamiliar to someone new. Could we add a brief explanation or a link to its documentation for context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added link
doc/repository-library-onboarding.md
Outdated
|
|
||
| ## Repository Setup: | ||
| 1) [Create ticket](http://go/onboard-repository-to-librarian) to onboard repository to Librarian automation. At a minimum, you should onboard to Tag and Release automation. | ||
| 2) Add `.librarian` directory to your repository with appropriate configuration files. See details [here](https://github.com/googleapis/librarian/blob/main/doc/language-onboarding.md#configuration-files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#configuration-files no longer exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks updated to correct link
| - For a monolithic repo remove the path entry for the library in your release-please-config.json and .release-please-manifest.json files | ||
| - For a single library repository, remove all the release-please config (.github/release-please.yml, release-please-config.json if it exists, .release-please-manifest.json if it exists) | ||
| 4) There is no requirement to stop using library-specific OwlBot post-processing files as part of this migration. However, while migrating, please open an issue in your generator repository for any improvements that could reduce your library's post-processing logic. | ||
| 4) There is no requirement to stop using library-specific post-processing files as part of this migration. However, any post processing should be included in a file named "librarian.<ext>", where <ext> corresponds to the script's file extension (e.g., "sh", "py"). While migrating, please also consider opening an issue in your generator repository for any improvements that could reduce your library's post-processing logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are post-processing files and how does this logic work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are changes that are made after protos are generated to that library, usually by hybrid clients with custom logic, ex additional wrapper methods they want added to generated files. For language containers, they can add logic to their generate method to check for these files and apply any post processor.
|  | ||
|
|
||
| ## Library Setup: | ||
| 1) Ensure all OwlBot PRs for that library have been merged and then release the library using a release-please PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section's focus on OwlBot and release-please is very specific to certain language ecosystems. Since other languages don't use these tools, I'd suggest splitting this part out to better differentiate these specific steps from information that is broadly relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separated into separate section
It's not immediately clear to me why this change is needed based on the current commit message. Would you mind adding some more context? |
Uh oh!
There was an error while loading. Please reload this page.