-
Notifications
You must be signed in to change notification settings - Fork 122
feat(api): retrieve app deployments based on last used data #7377
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 introduces a significant enhancement to the API by providing a new GraphQL query, 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
|
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7377.hive-storybook.pages.dev |
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 new activeAppDeployments GraphQL query to find stale deployments, which is a valuable addition for maintenance and cleanup. The implementation is accompanied by thorough integration tests and clear documentation. My main feedback is a performance concern regarding how deployments are fetched and filtered, which could be problematic for targets with a large number of deployments. I've also noted an opportunity to reduce code duplication in the new tests.
packages/services/api/src/modules/app-deployments/providers/app-deployments.ts
Show resolved
Hide resolved
💻 Website PreviewThe latest changes are available as preview in: https://pr-7377.hive-landing-page.pages.dev |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
7af3899 to
16aea1d
Compare
jdolle
left a comment
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.
Great job adding descriptions, comments, and docs. These are all very helpful. I'm excited for App Deployments to get more flushed out
| paginatedDeployments = filteredDeployments.filter(deployment => { | ||
| const deploymentCreatedAt = new Date(deployment.createdAt).getTime(); | ||
| return ( | ||
| deploymentCreatedAt < cursorCreatedAt || |
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.
I think it'd be simpler to build this into the sql query.
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 date filtering happens after the postgres query (using clickhouse data) so we can't paginate in sql without potentially skipping rows that would pass the filter
| AND "retired_at" IS NULL | ||
| ${args.filter.name ? sql`AND "name" ILIKE ${'%' + args.filter.name + '%'}` : sql``} | ||
| ORDER BY "created_at" DESC, "id" | ||
| LIMIT ${maxDeployments} |
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.
To avoid setting this limit and to simplify this logic, I wonder if we should be flip the query order. So that you first collect, from Clickhouse, a set of the deployments that match the lastUsed filter, and then use those IDs to fetch the rest of the details from postgres. This should eliminate the overfetching and avoid any limit issues for incredibly large deployment sets
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.
Flipping the order would severely complicate the logic, including table joins across clickhouse, which is an expensive task without the data from postgres. I do not think this is worth it, nor do I think there would be 1000 deployments(?), especially considering that the idea is to tidy old deployments
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 is best a follow up pr, as docs are deployed as time of merging the PR, before the code changes are deployed to production!
| activeAppDeployments( | ||
| first: Int | ||
| after: String | ||
| filter: ActiveAppDeploymentsFilter! | ||
| ): AppDeploymentConnection! |
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 corresponding types need to be annotated with @tag(name:"public"), so they are part of the public GraphQL API.
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.
See https://github.com/graphql-hive/console/pull/7377/files/16aea1d6a6e847a02f2178e50e89b4eb027bb17c#r2598351349 and https://github.com/graphql-hive/console/pull/7377/files/16aea1d6a6e847a02f2178e50e89b4eb027bb17c#r2598348711
Aside from that, the logic seems legit for, given the technical constraints of having to query two databases. If there is general thoughts on how we could improve this set up, lets start a discussion about it independently.
Description
Adds
activeAppDeploymentsGraphQL query to find app deployments based on usage criteria, enabling teams to identify and clean up stale deploymentsFilter options:
lastUsedBefore- find deployments used but not recentlyneverUsedAndCreatedBefore- find old unused deploymentsname- filter by app name (partial match)Closes CONSOLE-1504