Skip to content

feat: create gapic-node-processing library #6604

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Aug 27, 2025
Merged

feat: create gapic-node-processing library #6604

merged 25 commits into from
Aug 27, 2025

Conversation

sofisl
Copy link
Contributor

@sofisl sofisl commented Aug 15, 2025

see https://docs.google.com/document/d/1hxAtt0FaMp6vZX3Z57E62qqzvzsP3EzRE56N4ye7fzo/edit?tab=t.0#heading=h.xzptrog8pyxf

This library generates a combined gapic package from a CLI tool vs. as an owlbot function

@sofisl sofisl requested review from yoshi-approver and a team as code owners August 15, 2025 20:33
@snippet-bot Snippet Bot
Copy link

snippet-bot bot commented Aug 15, 2025 •
edited
Loading

Here is the summary of changes.

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

sofisl
sofisl commented Aug 15, 2025
View reviewed changes
@sofisl sofisl mentioned this pull request Aug 15, 2025
Merged
@sofisl sofisl mentioned this pull request Aug 15, 2025
Open
miguelvelezsa
miguelvelezsa reviewed Aug 18, 2025
View reviewed changes
packages/gapic-node-templating/src/commands/generate-combined-library.ts Outdated
* default-version: what is the default version of the library (default is highest)
*/
async handler(argv: CliArgs) {
const writeDestination = argv['destination-path'] || argv['library-path'];
Copy link
Contributor

Choose a reason for hiding this comment

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

add type. change variable name to destinationPath?

Copy link
Contributor

Choose a reason for hiding this comment

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

also I would explicitly ask for the destination-path, if not defined return error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See other comment re: user experience vs. our use case, but I think I'll leave it as is since that is the default behavior we want! I guess I'm thinking, I just want a "black-box" that just takes multiple versions of a library, and returns a single combined library in that same place. The real reason I added a destinationPath is just so that we could test it more easily.

packages/gapic-node-templating/src/commands/generate-combined-library.ts Outdated
try {
await combineLibraries(argv['library-path'], writeDestination);
} catch (err) {
if (!(err as any).message.includes('Unexpected library format')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not throw an error always?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking about cases where we've run the tool twice (once the library is well-formed). If that's the case, it won't have only top-level directories, it will also have files. The traverseDirectories function should fail, since it will encounter files. In that case, I don't think we should error, but just say that the library wasn't in the format we were expecting. I've updated it so that we're warning the user that we ran into this error.

packages/gapic-node-templating/src/combine-libraries.ts Outdated
* @param {Array<{filePath: string; content: string}>} files An array of objects, where each object has a file path and its content.
* @returns {Promise<void>} A promise that resolves when all files are written.
*/
export async function createDirectoryAndWriteFiles(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would split the directory creation and the files creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think actually this is a misnomer of a function. Really what we're doing is creating any directories that are required along the way to create a file. I've renamed the function to writeFilesToGivenLocation

@sofisl
Copy link
Contributor Author

sofisl commented Aug 26, 2025

Now this is a huge PR because I renamed the library >.<

@sofisl
Copy link
Contributor Author

sofisl commented Aug 26, 2025

need to merge: googleapis/repo-automation-bots#5922 first (or we can admin-merge, it's just to ignore owlbot)

@sofisl
Copy link
Contributor Author

sofisl commented Aug 26, 2025

Owlbot is failing because it's trying to check the template file:

+ python3 -m synthtool.languages.node_mono_repo
From https://github.com/googleapis/google-cloud-node
 * [new branch]            main       -> main
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/synthtool/synthtool/languages/node_mono_repo.py", line 599, in <module>
    owlbot_entrypoint()
    ~~~~~~~~~~~~~~~~~^^
  File "/synthtool/synthtool/languages/node_mono_repo.py", line 572, in owlbot_entrypoint
    owlbot_main(
    ~~~~~~~~~~~^
        dir,
        ^^^^
    ...<3 lines>...
        patch_staging,
        ^^^^^^^^^^^^^^
    )
    ^
  File "/synthtool/synthtool/languages/node_mono_repo.py", line 481, in owlbot_main
    open(Path(relative_dir, ".repo-metadata.json").resolve(), "rt")
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/workspace/google-cloud-node/packages/gapic-node-processing/templates/bootstrap-templates/.repo-metadata.json'

so this is safe to ignore

feywind
feywind previously approved these changes Aug 26, 2025
View reviewed changes
Copy link
Contributor

@feywind feywind left a comment

Choose a reason for hiding this comment

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

A lot of pedantic style stuff ^^; But hopefully there's something useful in there.

@sofisl
Copy link
Contributor Author

sofisl commented Aug 27, 2025

@feywind thank you so much for the indepth review, I really appreciate it! PTAL :)

@sofisl sofisl requested a review from feywind August 27, 2025 20:09
feywind
feywind approved these changes Aug 27, 2025
View reviewed changes
Copy link
Contributor

@feywind feywind left a comment

Choose a reason for hiding this comment

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

Re-approving!

@sofisl sofisl merged commit 5267f25 into main Aug 27, 2025
9 of 11 checks passed
@sofisl sofisl deleted the createCombinedLibrary branch August 27, 2025 20:56
@sofisl sofisl mentioned this pull request Aug 28, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miguelvelezsa miguelvelezsa miguelvelezsa left review comments

@feywind feywind feywind approved these changes

@yoshi-approver yoshi-approver Awaiting requested review from yoshi-approver yoshi-approver is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sofisl @miguelvelezsa @feywind