-
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
ci: attempt to enable new CI for security-center #3966
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 provide my feedback shortly. In the meantime, here's a summary of the changes for you and other reviewers to get up to speed quickly:
This pull request, authored by briandorsey, aims to enable a new CI for the security-center
samples. The changes are focused on CI configuration and don't modify the tests themselves.
Here's a breakdown of the changes:
- Added
ci-setup.json
: This file contains configuration settings (GCLOUD_ORGANIZATION and GOOGLE_SAMPLES_PROJECT) necessary for running the CI on the new runner. This file is located atsecurity-center/snippets/ci-setup.json
and contains the following:
{
"GCLOUD_ORGANIZATION": "1081635000895",
"GOOGLE_SAMPLES_PROJECT": "long-door-651"
}
- Updated
nodejs-dev.jsonc
: Thesecurity-center/snippets
directory was added to the list of samples included in the development CI configuration. The line was added between lines 196 and 197. - Updated
nodejs-prod.jsonc
: Thesecurity-center/snippets
directory was removed from the list of samples excluded in the production CI configuration. This was line 105, which was removed.
The author has indicated that tests haven't been modified, but I will verify this during my review. I'll also check the overall functionality and ensure the new CI setup works as intended.
A little haiku to brighten your day:
Code flows like a stream,
New CI builds, a steady hand,
Bugs flee in the night.
Thanks,
Gemini
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 adds CI configuration for the security-center/snippets
directory, enabling it to run under the new CI runner. This is a positive change that improves the reliability and automation of the test suite. The added ci-setup.json
file provides the necessary environment variables for the tests. The removal of security-center/snippets
from the exclude-packages
list in .github/config/nodejs-prod.jsonc
is also correct. However, the checklist items related to testing and linting haven't been completed. It's crucial to ensure tests pass and linting is clean before merging. I've also noticed a few minor style inconsistencies in the ci-setup.json
file. I'm referencing elements from the Samples Style Guide in this review.
keys set properly. Remove and try again to confirm.
This final test run looks a bit weird because I removed the ci-setup.json in the package directory since it's not needed, so those tests didn't run this time. They did run successfully on the last two checkins, though. |
Description
CI related config change. Adding ci-setup.json to support running these tests under the new CI runner. No changes to tests themselves.
Checklist
npm test
(see Testing)npm run lint
(see Style)GoogleCloudPlatform/nodejs-docs-samples
. Not a fork.