Skip to content

Move the samples over#8848

Draft
danieljbruce wants to merge 1 commit into
mainfrom
move-samples-from-spanner
Draft

Move the samples over#8848
danieljbruce wants to merge 1 commit into
mainfrom
move-samples-from-spanner

Conversation

@danieljbruce

Copy link
Copy Markdown
Contributor

CI experiment

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@product-auto-label product-auto-label Bot added the samples Issues that are directly related to samples. label Jul 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 introduces a comprehensive set of Node.js samples for Cloud Spanner, including archived versions, along with configuration files like .eslintrc.yml, package.json, and an auto-generated README.md. The review comments highlight several critical issues: incorrect handling of the exists() promise return value in backups-delete.js and instance-config-delete.js (which returns an array rather than a direct boolean), broken relative require paths in datatypes.js, and misplaced OpenTelemetry dependencies in package.json that should be under dependencies instead of devDependencies. Additionally, minor typos and a duplicate entry were identified in the README.md file.

await backup.delete();

// Verify backup no longer exists
const exists = await backup.exists();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The backup.exists() method returns a Promise<[boolean]>, so the exists variable will be an array (e.g., [true] or [false]). The condition if (exists) will always evaluate to true because an array is a truthy value. The check should be on the first element of the array.

  if (exists[0]) {

Comment on lines +611 to +616
const {updateWithNumericData} = require('../numeric-update-data');
const {queryWithNumericParameter} = require('../numeric-query-parameter');

const {addJsonColumn} = require('./json-add-column');
const {updateWithJsonData} = require('../json-update-data');
const {queryWithJsonParameter} = require('../json-query-parameter');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The require paths for the numeric and json samples are incorrect. They are pointing to the parent directory (../) instead of the current directory (./). This will cause the sample to fail when run.

Suggested change
const {updateWithNumericData} = require('../numeric-update-data');
const {queryWithNumericParameter} = require('../numeric-query-parameter');
const {addJsonColumn} = require('./json-add-column');
const {updateWithJsonData} = require('../json-update-data');
const {queryWithJsonParameter} = require('../json-query-parameter');
const {updateWithNumericData} = require('./numeric-update-data');
const {queryWithNumericParameter} = require('./numeric-query-parameter');
const {addJsonColumn} = require('./json-add-column');
const {updateWithJsonData} = require('./json-update-data');
const {queryWithJsonParameter} = require('./json-query-parameter');

console.log(`Deleting ${instanceConfig.id}...\n`);
await instanceConfig.delete();
// Verify that the instance config no longer exists
const exists = await instanceConfig.exists();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The instanceConfig.exists() method returns a Promise<[boolean]>, so the exists variable will be an array (e.g., [true] or [false]). The condition if (exists) will always evaluate to true because an array is a truthy value. The check should be on the first element of the array.

      if (exists[0]) {

Comment on lines +24 to +33
"devDependencies": {
"@google-cloud/opentelemetry-cloud-trace-exporter": "^2.4.1",
"@opentelemetry/exporter-trace-otlp-grpc": "^0.57.0",
"@opentelemetry/instrumentation": "^0.57.0",
"@opentelemetry/instrumentation-grpc": "^0.57.0",
"@opentelemetry/resources": "1.30.1",
"@opentelemetry/sdk-trace-base": "~1.30.1",
"@opentelemetry/sdk-trace-node": "^1.30.1",
"@opentelemetry/api": "^1.9.0",
"@opentelemetry/core": "^1.30.1",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The OpenTelemetry packages are used directly in the observability samples (observability-traces.js, observability-traces-otlp.js), so they should be listed under dependencies instead of devDependencies. This ensures they are installed for users who want to run these samples from the published package.

* [Add and drop new database role](#add-and-drop-new-database-role)
* [Backups-cancel](#backups-cancel)
* [Copies a source backup](#copies-a-source-backup)
* [Copies a source backup](#copies-a-source-backup)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There's a duplicate entry for "Copies a source backup" in the table of contents. This line is identical to the previous one.

__Usage:__


`node ppg-numeric-data-type.js <INSTANCE_ID> <DATABASE_ID> <PROJECT_ID>`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There appears to be a typo in the usage command. ppg-numeric-data-type.js should likely be pg-numeric-data-type.js.

Suggested change
`node ppg-numeric-data-type.js <INSTANCE_ID> <DATABASE_ID> <PROJECT_ID>`
node pg-numeric-data-type.js <INSTANCE_ID> <DATABASE_ID> <PROJECT_ID>

__Usage:__


`node table-create-with-foreign-key-delete-cascade.js.js <INSTANCE_ID> <DATABASE_ID> <PROJECT_ID>`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There's a typo in the filename in the usage command. It has a double .js.js extension.

Suggested change
`node table-create-with-foreign-key-delete-cascade.js.js <INSTANCE_ID> <DATABASE_ID> <PROJECT_ID>`
node table-create-with-foreign-key-delete-cascade.js <INSTANCE_ID> <DATABASE_ID> <PROJECT_ID>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant