-
Notifications
You must be signed in to change notification settings - Fork 2k
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: test-isolation package setup configs #3951
base: main
Are you sure you want to change the base?
Conversation
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.
Hi team,
I'm currently reviewing this pull request and will have a full review in a few minutes. In the meantime, here's a quick summary for you and other reviewers to get up to speed:
This pull request, authored by davidcavazos, aims to improve test isolation by introducing package setup configuration files. These configuration files allow packages to specify per-package settings such as secrets, environment variables, and runtime versions.
Here's a breakdown of the changes:
config.go
: This file has been significantly modified to support loading and handling of these new setup configuration files. Specifically:- Lines 24-29: Removed the
regexp
import, simplifying the code. - Lines 32-43: Added a
Setup
struct within theConfig
struct to handle the setup file's filename and default values. - Lines 48-57: Added a
Package
struct to represent a package with its path and setup configurations. - Lines 71-79: Simplified the
LoadConfig
function by using a helper function to read JSONC files. - Lines 156-185: The
Affected
function was updated to return an array ofPackage
structs, including the setup configurations.
- Lines 24-29: Removed the
utils.go
: A new helper functionReadJsonc
was added (lines 29-40) to handle reading and parsing JSONC files, simplifying the main config loading logic..github/config/nodejs-dev.jsonc
and.github/config/nodejs-prod.jsonc
: These configuration files were updated to include the newsetup
section, specifying asetup.json
file and default values..github/workflows/ci-dev.yaml
: This file was updated (lines 70-81) to use thegoogle-github-actions/get-secretmanager-secrets@v2
action to retrieve secrets defined in thesetup.json
files.generative-ai/snippets/setup.json
: A samplesetup.json
file was added, demonstrating how to define secrets and Node.js version.
Several files also had lines removed related to the previous comment handling logic, which is now handled by the new ReadJsonc
function. The functions/http/uploadFile
sample was also removed from the ignore list in the production config file.
I'll provide a more detailed review shortly. In the meantime, please let me know if you have any questions.
And now, a little haiku to brighten your day:
Tests run, green light shows,
Configs now make setup smooth,
Code flows, clean and bright.
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 pull request introduces support for per-package setup configuration files, which is a valuable addition. The implementation looks good overall, but there are a few points to address regarding consistency, error handling, and best practices.
|
||
packages := make([]Package, 0, len(changed)) | ||
for _, path := range changed { | ||
pkg := Package{Path: path, Setup: &c.Setup.Defaults} |
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.
Instead of taking the address of c.Setup.Defaults
, consider creating a deep copy of the map. This prevents unintended modifications to the default setup values if a package setup modifies the map it receives. A simple deep copy implementation for map[string]any
could be added as a helper function.
pkg := Package{Path: path, Setup: &c.Setup.Defaults} | |
pkg := Package{Path: path, Setup: deepCopy(c.Setup.Defaults)} |
Path string `json:"path"` | ||
|
||
// Package setup configurations. | ||
Setup *map[string]any `json:"setup"` |
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.
The Setup
field in the Package
struct is a pointer to a map. Consider making it a map directly (Setup map[string]any
) unless there's a specific reason for it to be a pointer. Using a map directly simplifies usage and avoids potential nil pointer dereferences. If the intent is to allow a nil value for setup, using a map directly still allows for that by checking if pkg.Setup == nil
.
Setup *map[string]any `json:"setup"` | |
Setup map[string]any `json:"setup"` |
err := ReadJsonc(setup_file, pkg.Setup) | ||
if err != nil { | ||
return nil, err | ||
} |
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.
Add error handling for when ReadJsonc fails. Currently, if ReadJsonc
returns an error, the error is not handled, and the execution continues, potentially leading to unexpected behavior. It's crucial to handle this error appropriately, either by logging it, returning it, or taking some other corrective action.
err := ReadJsonc(setup_file, pkg.Setup)
if err != nil {
fmt.Fprintf(log, "Error reading setup file %q: %v\n", setup_file, err)
return nil, err
}
…ples into test-isolation-configs
…ples into test-isolation-configs
4f00136
to
7119d52
Compare
7119d52
to
311055b
Compare
8389be9
to
37befa1
Compare
37befa1
to
9372772
Compare
Description
Supports package setup configuration files. These allow packages to provide per-package configurations like secrets, environment variables, and the runtime version.
Checklist
npm test
(see Testing)npm run lint
(see Style)GoogleCloudPlatform/nodejs-docs-samples
. Not a fork.