-
Notifications
You must be signed in to change notification settings - Fork 124
feat: protected app deployment retiring #7432
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @adambenhassen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant enhancement to app deployment management by introducing a robust retirement protection mechanism. It empowers users to safeguard active app deployments from inadvertent retirement through configurable rules based on usage patterns. The changes span across the API, CLI, and UI, providing a complete solution for managing and overriding these protections, backed by necessary database schema updates and comprehensive testing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a protection mechanism to prevent accidental retirement of in-use application deployments. The changes are comprehensive, spanning the database, backend API, CLI, and frontend UI. The implementation includes new GraphQL endpoints for configuration, enhanced error reporting with protection details, and a new settings page. The test coverage for both API and CLI scenarios is thorough. I have a couple of suggestions for improvement, one related to our architectural patterns for data access and another to make error messages more informative when multiple protection rules are violated.
packages/services/api/src/modules/app-deployments/providers/app-deployments.ts
Show resolved
Hide resolved
0a7bcdc to
ae97c6d
Compare
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-hive/apollo |
0.46.0-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/cli |
0.56.1-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/core |
0.19.0-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/envelop |
0.40.1-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/yoga |
0.46.1-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
hive |
8.14.0-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
hive-apollo-router-plugin |
2.3.6-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
hive-console-sdk-rs |
0.2.3-alpha-20251217165442-a06fbf10adf650bd431c2d236058f599b54c9f19 |
npm ↗︎ unpkg ↗︎ |
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7432.hive-storybook.pages.dev |
💻 Website PreviewThe latest changes are available as preview in: https://pr-7432.hive-landing-page.pages.dev |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
ae97c6d to
c50518b
Compare
c50518b to
a06fbf1
Compare
|
|
||
| const targetSlug = `${organization.slug}/${project.slug}/${target.slug}`; | ||
|
|
||
| // CLI retire without --force should fail |
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.
Nice job on these tests. I appreciate that you checked that it would fail without force before checking force passes.
| this.log(` Current traffic: ${details.currentTrafficPercentage.toFixed(2)}%`); | ||
| } | ||
| this.log(` Max traffic threshold: ${details.maxTrafficPercentage}%`); | ||
| this.log('\nUse --force to bypass protection.\n'); |
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.
Again great job. You provide the important details and explain how they can bypass.
| name: '2025.12.15T00-00-00.app-deployment-protection.ts', | ||
| run: ({ sql }) => sql` | ||
| ALTER TABLE targets ADD COLUMN app_deployment_protection_enabled BOOLEAN NOT NULL DEFAULT FALSE; | ||
| ALTER TABLE targets ADD COLUMN app_deployment_protection_min_days_inactive INT NOT NULL DEFAULT 30; |
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 may get explained as I review the rest of the PR. But how do the min days work with our usage retention_in_days limit? E.g. if we retain only 7 days but have min_days_inactive as 30, then after 7 days of not being used will we think this is satisfied?
| targetId: args.targetId, | ||
| appName: args.appDeployment.name, | ||
| appVersion: args.appDeployment.version, | ||
| periodDays: 30, |
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.
should this be configurable also?
| WHERE | ||
| "target" = ${args.targetId} | ||
| AND "timestamp" >= toDateTime(${formatClickHouseDate(periodFrom)}, 'UTC') | ||
| AND "timestamp" <= toDateTime(${formatClickHouseDate(periodTo)}, 'UTC') |
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 utility method seems like a lot of work that the client or clickhouse should handle for us.
I dug a little into clickhouse docs and I think a better approach is to use parseDateTimeBestEffort(${periodFrom.toISOString()}). parseDateTimeBestEffort handles ISO format strings and toISOString always returns UTC, so it should be equivalent.
| AND "timestamp" <= toDateTime(${formatClickHouseDate(periodTo)}, 'UTC') | ||
| `, | ||
| queryId: 'get-app-deployment-traffic-percentage', | ||
| timeout: 20_000, |
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.
Is 20s a default we use? My preference is to timeout quicker because 20s seems like a long time. If it's waiting that long then it's likely going to fail.
| Update the app deployment protection configuration of a target. | ||
| """ | ||
| updateTargetAppDeploymentProtectionConfiguration( | ||
| input: UpdateTargetAppDeploymentProtectionConfigurationInput! @tag(name: "public") |
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.
Minor: I prefer keeping new APIs private until we're sold on them. But if a customer is asking for this -- public is 👍
| """ | ||
| Input for updating app deployment protection configuration. | ||
| Fields not provided (omitted) will retain the previous value. |
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.
Do we error or return OK if no input fields are provided?
|
|
||
| const selector = await this.idTranslator.resolveTargetReference({ reference: args.target }); | ||
| if (!selector) { | ||
| this.session.raise('target:modifySettings'); |
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.
should the assertion be before the selection?
If you assert target:modifySettings before resolveTargetReference, then we can correctly return a more accurate error like "target doesnt exist"
| }, | ||
| validationSchema: Yup.object().shape({ | ||
| minDaysInactive: Yup.number() | ||
| .min(0, 'Must be at least 0') |
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.
should a max be determined by retentionInDays?
Description
Adds protection settings for app deployment retirement to prevent accidental removal of actively-used deployments.
How It Works
retireAppDeploymentis called and protection is enabled:minDaysInactivemaxTrafficPercentage--forcein CLI) bypasses all checksSettings page
Closes CONSOLE-1501