-
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: enable new CI for vision package #3968
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 provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed:
This pull request, authored by briandorsey, aims to enable a new CI configuration for the vision
package. The changes are primarily focused on CI configuration and don't modify the tests themselves.
Here's a breakdown of the changes:
- Added
vision
package to CI configuration:- The
vision
package was added to the list of packages included in the Node.js development (nodejs-dev.jsonc
) CI configuration (line 201). This ensures that thevision
package's tests are included in the development CI pipeline. - Conversely, the
vision
package was removed from the Node.js production (nodejs-prod.jsonc
) CI configuration (line 111). This suggests that the production CI pipeline will not include tests for thevision
package. This might be intentional, but it warrants a closer look.
- The
- Created
ci-setup.json
file: A new file,vision/ci-setup.json
, was created (it's currently empty). This file likely serves as a configuration file for the new CI setup for thevision
package. It's worth investigating why this file is empty at this stage.
Potential Issues to Consider:
- Empty
ci-setup.json
: The purpose of the emptyvision/ci-setup.json
file needs clarification. Is this a placeholder, or is there a reason for it being empty? - Production CI Exclusion: The removal of
vision
from the production CI configuration should be reviewed to ensure it's intentional and doesn't introduce any risks.
I'll provide a more detailed review shortly, but this summary should give everyone a good starting point.
And now, a little haiku to brighten your day:
Code flows like a stream,
Tests run, a green light appears,
Vision takes its flight.
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 enables a new CI for the vision package by adding a ci-setup.json
file and updating the CI configuration files. The changes are minimal and straightforward. However, the addition of an empty ci-setup.json
file raises questions about its purpose and potential future content. Additionally, there are opportunities to improve clarity and consistency in the configuration files.
I have referenced the following elements from the provided style guide:
- File naming:
ci-setup.json
follows the kebab-case convention used in other configuration files likenodejs-dev.jsonc
andnodejs-prod.jsonc
. - Configuration files: The use of JSON or JSONC for configuration files is consistent with the project's existing practice.
@@ -0,0 +1 @@ | |||
{} |
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 ci-setup.json
file is currently empty. Could you elaborate on its intended purpose and what configuration it will eventually contain? Adding a brief description as a comment within the file would be helpful. Per the style guide, documentation is important, even for configuration files.
{
// Configuration for CI setup for the vision package.
}
Description
Batch CI related config change. Adding ci-setup.json to support running these tests under the new CI runner. No changes to tests themselves.