chore(tests): Update infra script #16956
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several modifications primarily focused on enhancing the documentation within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16956 +/- ##
=======================================
Coverage 36.41% 36.41%
=======================================
Files 6898 6898
Lines 144881 144881
Branches 41407 41407
=======================================
Hits 52756 52756
Misses 92125 92125
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
infra/package.json (1)
39-40: Consider updating to a newer yarn versionWhile pinning the yarn version is good practice, version 3.2.3 is relatively old. Consider updating to the latest stable 3.x or 4.x version for security updates and improvements.
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
28-28: Update JSDoc to reflect requiredcwdparameterThe
cwdparameter is now required in the type interface, but the JSDoc still shows it as optional. This inconsistency should be addressed.- * @param {string} config.cwd - Working directory for the web server command. + * @param {string} config.cwd - Required. Working directory for the web server command.README.md (1)
78-78: LGTM! Consider using yarn workspace command instead.The subshell command for installing infrastructure dependencies is clear and functional. However, for consistency with monorepo practices, you might consider using yarn's workspace command instead:
-(cd infra/ && yarn install) +yarn workspace @island.is/infra installThis approach maintains consistency with yarn workspace conventions while achieving the same result.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
README.md(1 hunks)infra/README.md(3 hunks)infra/package.json(2 hunks)infra/src/cli/cli.ts(2 hunks)libs/testing/e2e/src/lib/config/playwright-config.ts(1 hunks)package.json(1 hunks)scripts/infra-cmds.js(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/infra-cmds.js
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 LanguageTool
infra/README.md
[style] ~1-~1: To make your text as clear as possible to all readers, do not use this foreign term. Possible alternatives are “below” or “further on” (in a document).
Context: # Infra Tool Documentation This CLI tool simpl...
(INFRA)
[style] ~13-~13: To make your text as clear as possible to all readers, do not use this foreign term. Possible alternatives are “below” or “further on” (in a document).
Context: ...ervices. ### Available Commands While infra offers various commands, one of the mos...
(INFRA)
🔇 Additional comments (4)
infra/package.json (1)
10-10: LGTM: Script rename aligns with project conventions
The rename from "cli" to "infra" maintains consistency with the broader changes in the project.
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
Line range hint 31-57: Verify impact of removing project configurations
The removal of the projects array from the configuration simplifies the setup but might impact test organization and filtering capabilities. Please ensure that:
- Test filtering requirements are met through alternative means (e.g., test tags or file organization)
- The simplified configuration still supports all necessary test scenarios across different NextJS apps
- Teams are aware of the new way to organize and run different test suites (smoke, acceptance, etc.)
infra/src/cli/cli.ts (2)
10-11: LGTM! CLI tool renaming is consistent.
The renaming from 'cli' to 'infra' aligns with the documentation updates and maintains consistency across the codebase.
133-134: Verify the necessity of these logging statements.
While the TTY and CWD information can be useful for debugging, consider:
- Are these logs necessary for all executions?
- Should they be behind a debug flag?
- Would structured logging (JSON format) be more appropriate for machine parsing?
Datadog ReportBranch report: ✅ 0 Failed, 333 Passed, 0 Skipped, 1m 14.13s Total Time |
svanaeinars
changed the title
Co-authored-by: Kristofer <kristofer@andes.is>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
infra/README.md (3)
Line range hint
1-12: LGTM! Consider adding installation instructions.The introduction and key features sections are well-structured and clear. Consider adding an "Installation" or "Getting Started" section before the Key Features to help new users get up and running quickly.
🧰 Tools
🪛 LanguageTool
[style] ~1-~1: To make your text as clear as possible to all readers, do not use this foreign term. Possible alternatives are “below” or “further on” (in a document).
Context: # Infra Tool Documentation This CLI tool simpl...(INFRA)
Line range hint
18-40: Consider adding examples for each command.While the commands are well-documented, adding simple usage examples for each command would make the documentation more user-friendly. For instance:
yarn infra render-env dev service-name yarn infra render-urls staging
49-49: Enhance the example with real-world use cases.Consider expanding the example to show common scenarios:
# Start a single service yarn infra run-local-env my-service # Start multiple services with dependencies yarn infra run-local-env my-service --dependencies=redis,postgres # Dry run to preview configuration yarn infra run-local-env my-service --dry
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
infra/README.md(3 hunks)infra/package.json(1 hunks)infra/src/cli/cli.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infra/package.json
🧰 Additional context used
🪛 LanguageTool
infra/README.md
[style] ~1-~1: To make your text as clear as possible to all readers, do not use this foreign term. Possible alternatives are “below” or “further on” (in a document).
Context: # Infra Tool Documentation This CLI tool simpl...
(INFRA)
🔇 Additional comments (3)
infra/README.md (1)
23-31: Clarify the difference between render-env-vars and render-local-env commands.
These commands appear to have identical descriptions. If they serve different purposes, please clarify the distinction. If they're aliases, consider documenting this or deprecating one in favor of the other.
infra/src/cli/cli.ts (2)
11-11: Script name update looks good
The script name change from 'yarn cli' to 'yarn infra' is consistent with the PR objectives and documentation updates.
Line range hint 108-108: New print option is well integrated
The --print option has been properly added to the run-local-env command with:
- Correct type definition as boolean
- Appropriate default value (false)
- Proper passing to the
runLocalServicesfunction
Also applies to: 116-116, 124-124
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
infra/README.md (1)
26-26: Consider enhancing the command documentation with examplesTo make the documentation more helpful, consider adding:
- A concrete example with actual service names
- Sample output or expected behavior
- Common option combinations and their use cases
Example enhancement:
yarn infra run-local-env <services> [options] + +# Example with specific services +yarn infra run-local-env auth-api web-app --dependencies redis,postgres + +# Common combinations +yarn infra run-local-env auth-api --dry --json # Preview configuration in JSON format +yarn infra run-local-env web-app --no-secrets --proxies # Development with proxies
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
infra/README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
infra/README.md
[style] ~1-~1: To make your text as clear as possible to all readers, do not use this foreign term. Possible alternatives are “below” or “further on” (in a document).
Context: # Infra Tool Documentation This CLI tool simpl...
(INFRA)
🔇 Additional comments (2)
infra/README.md (2)
Line range hint 1-11: Well-structured introduction and key features section!
The introduction clearly explains the tool's purpose, and the key features are well-organized with descriptive bullet points.
🧰 Tools
🪛 LanguageTool
[style] ~1-~1: To make your text as clear as possible to all readers, do not use this foreign term. Possible alternatives are “below” or “further on” (in a document).
Context: # Infra Tool Documentation This CLI tool simpl...
(INFRA)
11-16: LGTM! Changes align with previous review suggestions.
* chore(tests): Update infra script and remove playwright projects * Minor changes * Revert Playwright change * Update infra/package.json Co-authored-by: Kristofer <kristofer@andes.is> * Fix docs --------- Co-authored-by: Kristofer <kristofer@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Summary by CodeRabbit
Documentation
Chores
infradirectory context.New Features
--printoption to therun-local-envcommand for output control.