feat: regenerated google-monitoring-dashboard#8643
Conversation
|
Separated from #8641 due to failing tests |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request regenerates the Cloud Monitoring API client for Node.js, updating configuration files, README documentation, package dependencies, and generated client code. However, several issues were identified in the review: the updated OwlBot copy source path caused package.json to regenerate with incorrect metadata (downgraded version and incorrect repository), which will break the publishing pipeline. Additionally, depending on a release candidate of google-gax is risky, the Node.js engine version range uses a non-standard 'v' prefix, and some generated directory links in the README incorrectly use '/blob/main/' instead of '/tree/main/'.
| - source: /google/monitoring/dashboard/google-monitoring-dashboard-nodejs | ||
| dest: /owl-bot-staging/google-monitoring-dashboard |
There was a problem hiding this comment.
Changing the copy source path to /google/monitoring/dashboard/google-monitoring-dashboard-nodejs has caused the regenerated package.json to use incorrect metadata. Specifically, the package version was downgraded from 4.1.2 to 0.1.0, and the repository was changed from the monorepo (googleapis/google-cloud-node) to a standalone repository (googleapis/nodejs-dashboard). This will break the monorepo's release and publishing pipeline. Please verify if the source path is correct or if the generator templates need to be adjusted for the monorepo.
| }, | ||
| "dependencies": { | ||
| "google-gax": "^5.0.0" | ||
| "google-gax": "^5.1.1-rc.1" |
There was a problem hiding this comment.
Depending on a release candidate version (^5.1.1-rc.1) of google-gax in production dependencies is risky for a stable package. Please consider using a stable release version of google-gax if available, or update the generator configuration to target a stable version.
There was a problem hiding this comment.
This does indeed need a fix, should be ^5.0.7. Did we not get that change into the generator in time?
| "overrides": { | ||
| "@sinonjs/fake-timers": "15.2.1" | ||
| } | ||
| "node": ">=v18" |
There was a problem hiding this comment.
The v prefix in the Node.js engine version range (>=v18) is non-standard and can cause issues with strict semver parsers or package managers. It is recommended to use >=18 instead.
| "node": ">=v18" | |
| "node": ">=18" |
There was a problem hiding this comment.
Agreed, but we may need a template update?
| [homepage_samples]: https://github.com/googleapis/google-cloud-node/blob/main/packages/google-monitoring-dashboard/samples | ||
| [homepage_changelog]: https://github.com/googleapis/google-cloud-node/blob/main/packages/google-monitoring-dashboard/CHANGELOG.md | ||
| [homepage]: https://github.com/googleapis/google-cloud-node/blob/main/packages/google-monitoring-dashboard |
There was a problem hiding this comment.
The generated links for homepage_samples and homepage use /blob/main/ for directories, which should instead use /tree/main/ (since they are directories, not files). Please update the upstream generator or templates to correct these URLs so they don't get overwritten in future regenerations.
References
- Do not manually edit auto-generated files to fix typos or make other changes, as these edits will be overwritten during the next regeneration. Instead, apply the fixes upstream in the generator or templates.
There was a problem hiding this comment.
I have to agree with Gemini here.
feywind
left a comment
feywind
left a comment
There was a problem hiding this comment.
There's a bit much in here for me to approve this one.
| }, | ||
| "dependencies": { | ||
| "google-gax": "^5.0.0" | ||
| "google-gax": "^5.1.1-rc.1" |
There was a problem hiding this comment.
This does indeed need a fix, should be ^5.0.7. Did we not get that change into the generator in time?
| "jsdoc": "^4.0.4", | ||
| "jsdoc-fresh": "^4.0.0", | ||
| "jsdoc-region-tag": "^3.0.0", | ||
| "long": "^5.3.1", |
There was a problem hiding this comment.
This is causing the unit test failures.
| "overrides": { | ||
| "@sinonjs/fake-timers": "15.2.1" | ||
| } | ||
| "node": ">=v18" |
There was a problem hiding this comment.
Agreed, but we may need a template update?
| 1. [Select or create a Cloud Platform project][projects]. | ||
| 1. [Enable billing for your project][billing]. | ||
| 1. [Enable the Monitoring Dashboards API][enable_api]. | ||
| 1. [Enable the Cloud Monitoring API API][enable_api]. |
There was a problem hiding this comment.
Broken. I think we must've added API to the API name, and API is also in the template.
|
|
||
|
|
||
|
|
||
| This library is considered to be **stable**. The code surface will not change in backwards-incompatible ways |
| [homepage_samples]: https://github.com/googleapis/google-cloud-node/blob/main/packages/google-monitoring-dashboard/samples | ||
| [homepage_changelog]: https://github.com/googleapis/google-cloud-node/blob/main/packages/google-monitoring-dashboard/CHANGELOG.md | ||
| [homepage]: https://github.com/googleapis/google-cloud-node/blob/main/packages/google-monitoring-dashboard |
There was a problem hiding this comment.
I have to agree with Gemini here.
|
I'll have a quick look into understanding the comments raised here - in particular what the templates look like for the version of the generator used. |
|
The difference in package.json is (it seems) due to googleapis-gen not having a "combined" GAPIC library, only the one under v1. I believe the other OwlBot changes I've created have basically not included package.json. I'm less certain about how README.md is expected to be generated. We could just revert README.md from this PR as well. I believe whatever we do here will be relevant for capacityplanner as well (#8642) which has the same issues. |
2 participants
No description provided.