feat: support status pages and services [sc-23401] #1039
Conversation
|
@sorccu This is my draft PR I used locally to test the deploy command for status pages |
sorccu
force-pushed
the
antoinecoutellier/sc-23401/public-api-crud-resources-for-the-cli
branch
from
15d3bd2 to
11ca62a
Compare
WalkthroughThis pull request expands the CLI's construct capabilities to include status page management. It enhances the public API by adding new export statements and extends the project data interface to support additional properties. Two new files are introduced that define constructs for status pages and their associated services, including the necessary interfaces, classes, constructors, and synthesis methods to generate structured representations of these constructs. Changes
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
|
I think this might need some rework that would also require backend changes. Basically it's pretty unusual that the creation of cards is not done separately as e.g. a hypothetical const bigService = new StatusPageService('foo-service-big', {
name: 'Big service',
})
const smallService = new StatusPageService('foo-service-small', {
name: 'Small service',
})
new StatusPage('foo-page', {
name: 'Dummy page',
url: 'just-a-dummy-page',
cards: [ // Magically creates card instances!
{
name: 'Card 1',
services: [
bigService,
smallService,
],
},
{
name: 'Card 2',
services: [
bigService,
],
},
],
})I can see from the models that a card can only belong to a single I think ideally the card model wouldn't be directly tied to a With no model changes, I think this would be a more consistent approach with the way the CLI currently does things: const dummyPage = new StatusPage('foo-page', {
name: 'Dummy page',
url: 'just-a-dummy-page',
})
const bigService = new StatusPageService('foo-service-big', {
name: 'Big service',
})
const smallService = new StatusPageService('foo-service-small', {
name: 'Small service',
})
new StatusPageCard('foo-card-1', {
name: 'Card 1',
statusPage: dummyPage,
services: [
bigService,
smallService,
],
})
new StatusPageCard('foo-card-2', {
name: 'Card 2',
statusPage: dummyPage,
services: [
bigService,
],
})Unfortunately the If implemented, this approach would require the introduction of a new Alternatively with some model changes, the following would be possible: const bigService = new StatusPageService('foo-service-big', {
name: 'Big service',
})
const smallService = new StatusPageService('foo-service-small', {
name: 'Small service',
})
const cardOne = new StatusPageCard('foo-card-1', {
name: 'Card 1',
services: [
bigService,
smallService,
],
})
const cardTwo = new StatusPageCard('foo-card-2', {
name: 'Card 2',
services: [
bigService,
],
})
new StatusPage('foo-page', {
name: 'Dummy page',
url: 'just-a-dummy-page',
cards: [
cardOne,
cardTwo,
],
})Here the benefit is that all links are pointing "downwards" to child resources instead of reaching upwards to parent resources. And it would be possible to reuse a card on multiple pages (though that would likely require some additional UI work to implement safely). |
My 2 cents. The "cards" resource is just for creating UI elements. They are strictly hierarchical. They should be nested under the status page resource as that is how they are used. Reusing cards is a 0.01% problem. And if it happens, it's actually easy to solve with TS in the MaC codebase. So I would vote to now abstract this more than needed and just use the implementation as provided by @shiini2 This code totally makes sense to me as cards are "just" a UI element we need to order. |
![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
🧹 Nitpick comments (3)
packages/cli/src/constructs/status-page-service.ts (1)
34-38: Consider adding type safety to the synthesize method return valueThe synthesize method returns
any|nullwhich works but lacks type safety. Consider defining a specific interface for the return type to improve type checking and documentation.- synthesize (): any|null { + interface StatusPageServicePayload { + name: string; + } + + synthesize (): StatusPageServicePayload|null {packages/cli/src/constructs/status-page.ts (2)
91-107: Consider more robust handling of optional fields in the synthesize methodThe synthesize method returns
nullfor optional fields that aren't provided. This approach may lead to cluttered payloads withnullvalues. Consider excluding these fields entirely when they're undefined.A more compact approach would be:
synthesize (): any|null { - return { - name: this.name, - url: this.url, - customDomain: this.customDomain, - logo: this.logo, - redirectTo: this.redirectTo, - favicon: this.favicon, - defaultTheme: this.defaultTheme, - cards: this.cards.map(card => ({ - name: card.name, - services: card - .services - ?.map((service) => Ref.from(service.logicalId)), - })), - } + const payload: any = { + name: this.name, + url: this.url, + cards: this.cards.map(card => { + const cardPayload: any = { + name: card.name, + } + + if (card.services?.length) { + cardPayload.services = card.services.map(service => Ref.from(service.logicalId)) + } + + return cardPayload + }), + } + + if (this.customDomain) payload.customDomain = this.customDomain + if (this.logo) payload.logo = this.logo + if (this.redirectTo) payload.redirectTo = this.redirectTo + if (this.favicon) payload.favicon = this.favicon + if (this.defaultTheme) payload.defaultTheme = this.defaultTheme + + return payload }
100-105: Consider validating that required service objects existWhen mapping services to references, there's no validation that the services actually exist. This could lead to cryptic errors if a service is undefined or doesn't have a logicalId.
cards: this.cards.map(card => ({ name: card.name, services: card .services - ?.map((service) => Ref.from(service.logicalId)), + ?.map((service) => { + if (!service || !service.logicalId) { + throw new Error(`Invalid service reference in card "${card.name}". Service must be a valid StatusPageService instance.`) + } + return Ref.from(service.logicalId) + }), })),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/cli/src/constructs/index.ts(1 hunks)packages/cli/src/constructs/project.ts(4 hunks)packages/cli/src/constructs/status-page-service.ts(1 hunks)packages/cli/src/constructs/status-page.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/cli/src/constructs/status-page-service.ts (1)
packages/cli/src/constructs/project.ts (1)
Session(144-219)
packages/cli/src/constructs/project.ts (2)
packages/cli/src/constructs/status-page.ts (1)
StatusPage(57-108)packages/cli/src/constructs/status-page-service.ts (1)
StatusPageService(14-39)
packages/cli/src/constructs/status-page.ts (2)
packages/cli/src/constructs/status-page-service.ts (1)
StatusPageService(14-39)packages/cli/src/constructs/project.ts (1)
Session(144-219)
🔇 Additional comments (11)
packages/cli/src/constructs/index.ts (1)
32-33: Exports look good!The new exports for status page functionality are consistent with the existing export pattern in the file. This correctly makes the new constructs available to users of the package.
packages/cli/src/constructs/project.ts (4)
11-11: Import of the new constructs looks goodThe new constructs have been properly added to the import list, maintaining the alphabetical ordering.
37-38: New ProjectData interface properties look goodThe additions to the ProjectData interface properly define the data structures for the new status page and service constructs, following the existing pattern.
55-56: Initialization of new properties in ProjectData looks goodThe new properties are correctly initialized as empty objects, matching the pattern used for other resource types.
107-108: Consider the ordering of status page resources in the synthesize methodThe current implementation synthesizes the status page services before the status pages themselves. This is functionally correct, but it's worth noting that status pages reference services, so this order ensures that services exist before they're referenced.
packages/cli/src/constructs/status-page-service.ts (2)
1-9: StatusPageServiceProps interface is correctly definedThe interface has proper documentation and defines the required name property for status page services.
11-32: StatusPageService constructor implementation looks goodThe StatusPageService class correctly extends Construct, defines the necessary type, and initializes properties in the constructor. The Session.registerConstruct call ensures the service is properly registered with the project.
packages/cli/src/constructs/status-page.ts (4)
6-15: StatusPageCardProps interface correctly defines card propertiesThe interface properly defines the name property as required and services as optional, with appropriate documentation.
17-17: StatusPageTheme type is well-definedThe StatusPageTheme type correctly uses string literals to define the possible theme values.
19-52: StatusPageProps interface is well-structuredThe interface defines all necessary properties with good documentation. The required properties (name, url, cards) are correctly marked as required, while optional properties use the optional modifier.
54-89: StatusPage class constructor handles all properties correctlyThe StatusPage class appropriately extends Construct, defines all properties, and initializes them in the constructor. The call to Session.registerConstruct ensures proper registration.
sorccu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cli/e2e/__tests__/deploy.spec.ts (1)
248-250: Good addition of status page constructs to deployment testing.The test now correctly verifies that the new StatusPage and StatusPageService constructs are properly recognized during deployment preview. This ensures consistent test coverage for the new functionality being added.
Consider adding more detailed tests that verify the specific behaviors and properties of status pages and services, not just their presence in the deployment output. Also, it's interesting that these constructs appear in the first configuration but not the second - this difference might warrant documentation if it represents an intentional configuration option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/e2e/__tests__/deploy.spec.ts(1 hunks)packages/cli/e2e/__tests__/fixtures/deploy-project/status-page.check.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/cli/e2e/tests/fixtures/deploy-project/status-page.check.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
I hereby confirm that I followed the code guidelines found at engineering guidelines
Affected Components
Notes for the Reviewer
New Dependency Submission