-
Notifications
You must be signed in to change notification settings - Fork 61
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
chore(dsl): Enforce types #16375
chore(dsl): Enforce types #16375
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16375 +/- ##
=======================================
Coverage 36.73% 36.73%
=======================================
Files 6809 6809
Lines 141086 141086
Branches 40224 40224
=======================================
Hits 51825 51825
Misses 89261 89261
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
WalkthroughThe changes involve updates to TypeScript type definitions and method signatures across several files to improve type safety and clarity. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebService
participant ServiceBuilder
participant Ingress
Client->>WebService: Request service setup
WebService->>ServiceBuilder: Initialize with ingress configuration
ServiceBuilder->>Ingress: Validate ingress mapping
Ingress-->>ServiceBuilder: Return validated ingress
ServiceBuilder-->>WebService: Return service setup
WebService-->>Client: Respond with service setup
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
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
infra/src/dsl/types/charts.ts (1)
39-39
: Improved clarity inEnvironmentServices
type declarationThe renaming of the mapped type parameter from
name
toenv
enhances the clarity and expressiveness of the DSL syntax. This change makes the type declaration more self-explanatory, as it now clearly indicates that the keys represent environments (OpsEnv
).Consider applying similar naming improvements to other type declarations in this file for consistency, such as the
EnvironmentConfigs
type on line 41. For example:export type EnvironmentConfigs = { [envName in OpsEnvName]: EnvironmentConfig }This would maintain a consistent naming convention throughout the file.
apps/web/infra/web.ts (2)
1-1
: Approved: Type assertion improves type safetyThe addition of the
Ingress
type import and the type assertion for theingress
configuration enhances type safety, which aligns with the PR objective. This change follows TypeScript best practices and improves code maintainability.Consider using a type annotation instead of a type assertion for better type safety:
primary: { // ... other properties } satisfies IngressThis approach allows TypeScript to verify that the object conforms to the
Ingress
type, rather than forcing it to be treated as that type.Also applies to: 53-53
Line range hint
1-72
: Overall structure and practices are well-implementedThe file demonstrates good practices for infrastructure-as-code:
- It uses a custom DSL for service configuration, which promotes consistency and reusability.
- The configuration covers essential aspects like environment variables, secrets, ingress, health checks, and resource limits.
- TypeScript is used effectively to improve type safety in the infrastructure setup.
While this file doesn't contain direct NextJS code, it sets up the infrastructure that could support a NextJS application. The configuration allows for environment-specific settings, which is a good practice for managing different deployment environments.
Consider documenting the purpose and usage of this infrastructure setup, either in this file or in a separate README, to help other developers understand how it relates to the NextJS application it supports.
infra/src/dsl/types/input-types.ts (3)
109-110
: Enhanced type safety with new IngressType and IngressMappingThe introduction of
IngressType
andIngressMapping
types improves the DSL's expressiveness and type safety for Kubernetes ingress resources. This change allows for more precise and self-documenting ingress configurations.Consider adding a brief comment explaining the purpose and usage of these new types to further improve the DSL's documentation.
138-138
: Enhanced clarity in Ingress interfaceThe changes in the Ingress interface improve semantic clarity and maintain consistency with previous modifications:
- Changing
[name in OpsEnv]
to[env in OpsEnv]
in thehost
property better represents the environment-specific nature of the configurations.- Updating
extraAnnotations
to use[env in OpsEnv]
and[annotation: string]
provides more clarity about the structure and purpose of the annotations.These changes enhance the readability and maintainability of the DSL.
Consider adding a brief comment explaining the purpose of
extraAnnotations
to further improve the DSL's documentation.Also applies to: 142-144
Line range hint
21-234
: Overall improvements in DSL clarity and type safetyThe changes in this file significantly enhance the DSL's clarity, type safety, and expressiveness:
- Consistent use of
env
instead ofidx
orname
in index signatures improves semantic clarity.- Introduction of
IngressType
andIngressMapping
types provides better structure for ingress configurations.- Updates to various interfaces and types (PostgresInfo, RedisInfo, Ingress, ExtraValues) maintain consistency and improve readability.
These modifications align well with Kubernetes and Helm concepts, making the DSL more robust and easier to use.
To further improve the DSL:
- Consider adding brief comments explaining the purpose and usage of new types and complex properties.
- Create or update documentation that outlines how to use this DSL to create complex Helm values, especially focusing on the new ingress configuration structure.
- If not already present, consider adding examples in the repository that demonstrate how to use these types to define services and their associated resources.
infra/src/dsl/dsl.ts (1)
Line range hint
1-569
: Improved type safety enhances DSL clarityThe changes to enforce typing for the
ingress
method significantly improve the clarity of the DSL by making the expected structure of ingress configurations explicit. This enhancement maintains the DSL's expressiveness while ensuring better integration with Kubernetes resources.However, to fully meet the coding guidelines for this directory, consider adding documentation on how to use this DSL to create complex Helm values, particularly focusing on the
ingress
configuration.Consider adding a code comment or JSDoc above the
ingress
method to explain its usage and provide an example of a validIngressMapping
object. This would help developers understand how to use this method to create complex Helm values for ingress configurations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/web/infra/web.ts (2 hunks)
- infra/src/dsl/dsl.ts (3 hunks)
- infra/src/dsl/types/charts.ts (1 hunks)
- infra/src/dsl/types/input-types.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/infra/web.ts (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
infra/src/dsl/dsl.ts (1)
Pattern
infra/src/dsl/**/*
: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
infra/src/dsl/types/charts.ts (1)
Pattern
infra/src/dsl/**/*
: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
infra/src/dsl/types/input-types.ts (1)
Pattern
infra/src/dsl/**/*
: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
🔇 Additional comments (6)
infra/src/dsl/types/input-types.ts (4)
21-21
: Improved semantic clarity in PostgresInfo typeThe change from
[idx in OpsEnv]
to[env in OpsEnv]
enhances the readability and self-documentation of the code. It clearly indicates that the keys represent different environments, which aligns well with the DSL's focus on infrastructure and deployment.
38-38
: Consistent naming in RedisInfo typeThe change from
[idx in OpsEnv]
to[env in OpsEnv]
in the RedisInfo type is consistent with the previous change in PostgresInfo. This consistency improves the overall readability and maintainability of the DSL.
117-117
: Improved type safety in ServiceDefinitionThe change from
{ [name: string]: Ingress }
toIngressMapping
for theingress
property enhances type safety and provides a more structured approach to defining ingress configurations. This improvement aligns well with Kubernetes resource definitions and makes the DSL more robust.
234-234
: Consistent naming in ExtraValues typeThe change from
[idx in OpsEnv]
to[env in OpsEnv]
in the ExtraValues type maintains consistency with previous modifications. This change enhances the readability of the DSL by explicitly indicating that the keys represent different environments.infra/src/dsl/dsl.ts (2)
20-20
: LGTM: Import ofIngressMapping
typeThe addition of the
IngressMapping
import aligns well with the PR objective of enforcing types for mappings. This change enhances type safety for ingress configurations.
465-465
: LGTM: Updatedingress
method signatureThe change to use
IngressMapping
instead of a generic object type enhances type safety and aligns with the PR objective. This update enforces proper typing for ingress configurations.To ensure this change doesn't break existing code, please run the following script to check for any incompatible usages of the
ingress
method:
Merging into #16405 |
Make mappings be typed, not arbitrary keys
Summary by CodeRabbit
New Features
IngressType
andIngressMapping
.Improvements
These changes aim to improve the reliability and maintainability of the application while enhancing user experience through clearer configurations.