Skip to content
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

Add insights for terraform plans #1604

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

michaeljguarino
Copy link
Member

@michaeljguarino michaeljguarino commented Nov 21, 2024

These are often hard to interpret too so an insight could be useful to have, will also see if i can send them w/ the pr comments

Fixes PROD-2913

Test Plan

unit tests

Checklist

  • If required, I have updated the Plural documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a meaningful title and summary to convey the impact of this PR to a user.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced stack_state field in AI insights, providing additional context.
    • Added insight field to stack_state object for detailed stack information.
    • Implemented new association between AiInsight and StackState.
  • Bug Fixes

    • Corrected minor typos in test descriptions.
  • Tests

    • Added new tests for handling StackRunUpdated events in a pending approval state.

@michaeljguarino michaeljguarino added the enhancement New feature or request label Nov 21, 2024
@michaeljguarino michaeljguarino requested a review from a team November 21, 2024 22:17
Copy link

linear bot commented Nov 21, 2024

Copy link
Contributor

coderabbitai bot commented Nov 21, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes in this pull request introduce several enhancements primarily focused on integrating the StackState with the AiInsight structure and related components. A new field StackState is added to the AiInsight struct, and various methods are implemented to facilitate insights and data retrieval for stack states. Additionally, GraphQL schema modifications include new fields to enhance data context. A migration is introduced to add an insight_id to the stack_states table, establishing a relationship with the ai_insights table.

Changes

File Path Change Summary
go/client/models_gen.go Added field StackState *StackState \json:"stackState,omitempty"`toAiInsight` struct.
lib/console/ai/evidence/stack_run.ex Modified fetch_code/1 to use a dynamic string based on run.type.
lib/console/ai/evidence/stack_state.ex Introduced implementation of Console.AI.Evidence protocol for StackState, adding methods: generate/1, insight/1, preload/1, and private methods state_description/1, fetch_code/1.
lib/console/ai/pubsub/protocol.ex Added implementation of Console.AI.PubSub.Insightful for StackRunUpdated, with a new method resource/1 for handling :pending_approval status.
lib/console/graphql/ai.ex Added field stack_state to ai_insight object, reformatted cluster_insight_component.
lib/console/graphql/deployments/stack.ex Added field insight to stack_state object, resolved using dataloader(Deployments).
lib/console/schema/ai_insight.ex Added has_one :stack_state, StackState, foreign_key: :insight_id to AiInsight schema.
lib/console/schema/stack_state.ex Added belongs_to :insight, AiInsight and updated @valid attribute to include insight_id.
priv/repo/migrations/20241121212536_add_insight_id_states.exs Introduced migration to add insight_id column to stack_states table.
schema/schema.graphql Added field stackState: StackState to AiInsight type in GraphQL schema.
test/console/ai/pubsub/consumer_test.exs Added new test module Console.AI.PubSub.ConsumerSyncTest and a test case for handling StackRunUpdated event in pending_approval state. Minor corrections made to existing tests.

Possibly related PRs

Suggested labels

frontend

Suggested reviewers

  • jsladerman
  • maciaszczykm

Poem

In the burrows deep, where insights bloom,
A stack's state whispers, dispelling the gloom.
With fields anew, our data takes flight,
In the realm of code, we hop with delight! 🐇✨
Each change a treasure, each line a new song,
Together we build, where all insights belong!


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (11)
priv/repo/migrations/20241121212536_add_insight_id_states.exs (1)

4-8: Consider adding an index for the foreign key.

Since this column will be used in JOIN operations between stack_states and ai_insights, adding an index would improve query performance.

Here's how you can add the index:

 def change do
   alter table(:stack_states) do
     add :insight_id, references(:ai_insights, type: :uuid, on_delete: :nilify_all)
   end
+  create index(:stack_states, [:insight_id])
 end
lib/console/schema/stack_state.ex (1)

30-33: Consider simplifying the insight association handling

The foreign key constraints are correctly added, but the cast_assoc for the insight relationship might be unnecessary since it's a belongs_to association. Typically, cast_assoc is more useful for has_many/has_one relationships where you need to handle nested attributes.

Consider simplifying to:

-    |> cast_assoc(:insight)

Unless you have a specific need to handle nested insight attributes in the same changeset, removing this line would simplify the code without losing functionality.

lib/console/ai/pubsub/protocol.ex (1)

32-37: Consider performance implications of synchronous preload

The synchronous database preload operation in the protocol implementation could impact performance, especially in high-throughput scenarios.

Consider using async preload or moving the database operation earlier in the pipeline:

-    case Console.Repo.preload(run, [:state]) do
+    # Option 1: Use async preload
+    case Console.Repo.preload(run, [:state], force: false) do

+    # Option 2: Ensure state is preloaded before reaching this point
+    case run do
lib/console/ai/evidence/stack_state.ex (2)

13-13: Consider optimizing preload performance.

The preload operation loads multiple associations ([:insight, run: [:stack, :cluster, :errors, :repository]]). Consider:

  1. Making the preload selective based on what data is actually needed
  2. Adding documentation about the performance implications

15-31: Consider externalizing template strings and adding nil checks.

Two suggestions for improvement:

  1. Move the static template text to a module attribute or external template file for better maintainability
  2. Add nil checks for run.repository and run.git to prevent runtime errors
+  @description_template """
+  The Plural stack %{stack_name} has a terraform plan generated and the user will want to understand what it means, in particular:
+
+  * expected blast radius of the change
+  * if any critical systems can be affected by the change
+  * whether it's safe to apply
+
+  The plan itself is recorded below:
+
+  ```
+  %{plan}
+  ```
+
+  It is sourcing %{type} configuration from the git repository at %{repo_url} from the folder %{folder} at ref %{ref}.
+  """
+
   defp state_description(%StackState{run: %StackRun{} = run} = state) do
+    with repo_url <- get_in(run, [:repository, :url]) || "unknown",
+         folder <- get_in(run, [:git, :folder]) || "unknown",
+         ref <- get_in(run, [:git, :ref]) || "unknown" do
     {:user, """
-    The Plural stack #{run.stack.name} has a terraform plan generated...
+    #{String.replace(@description_template, [
+        "%{stack_name}", run.stack.name,
+        "%{plan}", state.plan,
+        "%{type}", run.type,
+        "%{repo_url}", repo_url,
+        "%{folder}", folder,
+        "%{ref}", ref
+      ])}
     """}
+    end
   end
lib/console/schema/ai_insight.ex (1)

30-30: Good architectural decision for Terraform plan insights

The one-to-one relationship between AiInsight and StackState is a clean way to model the interpretation of Terraform plans. This design:

  • Maintains clear separation between the plan state and its interpretation
  • Allows for future extensibility (e.g., adding more insight types)
  • Makes it easy to track and manage the lifecycle of insights
test/console/ai/pubsub/consumer_test.exs (2)

Line range hint 47-47: Fix typos in test descriptions

There are typos in two test descriptions where "faield" should be "failed".

-    test "it will not run if the run isn't faield" do
+    test "it will not run if the run isn't failed" do

-    test "it will not stack if the stack isn't faield" do
+    test "it will not stack if the stack isn't failed" do

Also applies to: 82-82


99-112: Consider enhancing test coverage

While the test effectively verifies the basic flow, consider adding:

  1. Assertions about the plan content being passed to OpenAI
  2. Multiple test cases with different plan contents
  3. Verification of error handling scenarios

Example enhancement:

test "it handles different types of terraform plans" do
  # Test setup similar to existing test
  plans = [
    {
      "plan with resource creation",
      "some plan with resource creation",
      "expected insight about creation"
    },
    {
      "plan with resource deletion",
      "some plan with resource deletion",
      "expected insight about deletion"
    }
  ]
  
  for {name, plan, expected} <- plans do
    expect(Console.AI.OpenAI, :completion, fn _, content ->
      assert content =~ plan
      {:ok, expected}
    end)
    
    # Test assertions
  end
end

test "it handles OpenAI errors gracefully" do
  # Test setup
  expect(Console.AI.OpenAI, :completion, fn _, _ -> 
    {:error, "API error"}
  end)
  
  # Verify error handling
end
lib/console/graphql/ai.ex (1)

95-95: LGTM! Consider adding field documentation.

The addition of the stack_state field is well-structured and consistent with the schema's patterns. Consider adding a @desc documentation string to clarify the field's purpose in relation to Terraform plan insights.

-    field :stack_state,       :stack_state,          resolve: dataloader(Deployments)
+    @desc "Associated stack state containing Terraform plan information"
+    field :stack_state,       :stack_state,          resolve: dataloader(Deployments)
lib/console/graphql/deployments/stack.ex (1)

335-338: Consider standardizing insight field descriptions across objects.

The insight field appears in multiple objects (stack, stack_run, stack_state) with slightly different descriptions. Consider documenting the relationship between insights and different contexts to help developers understand when and how insights are generated.

Example documentation pattern:

 field :insight, :ai_insight, resolve: dataloader(Deployments),
-      description: "an insight explaining the state of this stack state, eg the terraform plan it represents"
+      description: """
+      AI-generated insight explaining this stack state.
+      For Terraform plans, provides interpretation of proposed changes.
+      Related to insights on stack_run (execution context) and stack (overall context).
+      """
schema/schema.graphql (1)

6198-6199: LGTM! Consider adding field documentation.

The addition of stackState field to AiInsight type is well-structured and aligns with the PR's objective of enhancing terraform plan interpretability. Consider adding a GraphQL description string to document the field's purpose.

  clusterInsightComponent: ClusterInsightComponent

+  "The stack state this insight was generated from, if any"
  stackState: StackState

  insertedAt: DateTime
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3c5e112 and a7f344a.

⛔ Files ignored due to path filters (3)
  • assets/src/generated/graphql-kubernetes.ts is excluded by !**/generated/**
  • assets/src/generated/graphql-plural.ts is excluded by !**/generated/**
  • assets/src/generated/graphql.ts is excluded by !**/generated/**
📒 Files selected for processing (11)
  • go/client/models_gen.go (1 hunks)
  • lib/console/ai/evidence/stack_run.ex (1 hunks)
  • lib/console/ai/evidence/stack_state.ex (1 hunks)
  • lib/console/ai/pubsub/protocol.ex (1 hunks)
  • lib/console/graphql/ai.ex (1 hunks)
  • lib/console/graphql/deployments/stack.ex (1 hunks)
  • lib/console/schema/ai_insight.ex (2 hunks)
  • lib/console/schema/stack_state.ex (2 hunks)
  • priv/repo/migrations/20241121212536_add_insight_id_states.exs (1 hunks)
  • schema/schema.graphql (1 hunks)
  • test/console/ai/pubsub/consumer_test.exs (1 hunks)
🔇 Additional comments (18)
priv/repo/migrations/20241121212536_add_insight_id_states.exs (1)

1-9: LGTM! The migration is well-structured.

The migration correctly implements the foreign key relationship between stack_states and ai_insights tables with appropriate UUID type and nilify strategy for deletions.

lib/console/schema/stack_state.ex (3)

3-3: LGTM: Appropriate module alias added

The AiInsight alias is correctly added alongside other schema aliases.


23-23: LGTM: Valid attributes updated correctly

The insight_id is appropriately added to the list of valid attributes, maintaining consistency with the new association.


16-18: Verify the database schema supports this new association

The association declarations look correct. The belongs_to relationships are properly defined for stack, run, and the new insight association.

Let's verify the database schema supports these associations:

✅ Verification successful

Database schema correctly supports the new association

The database schema fully supports all the declared associations:

  • Initial migration (20240402221806) created stack_states with stack_id and run_id foreign keys
  • Recent migration (20241121212536) added insight_id foreign key with appropriate reference to ai_insights table
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the database schema has the required foreign key columns
# and if there are any existing migrations that might conflict

# Look for related migration files
fd -e exs . "priv/repo/migrations" | xargs rg -l "stack_states|ai_insights"

# Look for schema definitions to verify foreign keys
rg "create.*table.*stack_states|alter.*table.*stack_states" -A 10

Length of output: 1914

lib/console/ai/pubsub/protocol.ex (2)

29-39: Implementation aligns well with PR objectives

The implementation correctly handles the stack run state for pending approvals, which is essential for providing insights into Terraform plans. The pattern matching and return values follow the protocol specification correctly.


29-39: Verify status handling across implementations

Let's ensure there's no overlap in status handling between different implementations.

✅ Verification successful

Let me gather more context about the protocol implementations to ensure we have a complete picture.


No status handling overlap found across implementations

Each implementation handles distinct status cases without any conflicts:

  • ServiceUpdated: handles stale and failed statuses for Service
  • StackRunUpdated: handles pending_approval status for StackRun
  • StackRunCompleted: handles failed status for StackRun
  • StackUpdated: handles failed status for Stack

The implementations are well-segregated based on both the event type and status, with no overlapping conditions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential overlapping status handling across implementations

# Search for all status pattern matches in protocol implementations
echo "Checking status patterns in protocol implementations:"
rg -A 1 "status: [:]?\w+" lib/console/ai/pubsub/

Length of output: 915


Script:

#!/bin/bash
# Get the protocol definition and all its implementations
echo "Protocol definition and implementations:"
ast-grep --pattern 'defprotocol Console.AI.PubSub.Insightful do
  $$$
end'

echo -e "\nAll protocol implementations:"
ast-grep --pattern 'defimpl Console.AI.PubSub.Insightful, for: $_'

Length of output: 3498

lib/console/ai/evidence/stack_state.ex (2)

1-7: LGTM! Well-structured protocol implementation.

The implementation follows Elixir best practices with clear module organization and proper use of aliases.


1-41: Verify integration with PR comment system.

The implementation successfully addresses the PR objective of enhancing Terraform plan interpretability. However, since the PR mentions "sending these insights along with pull request comments", we should verify this integration.

✅ Verification successful

Based on the code evidence gathered, I can now generate the final response:

Integration with PR comments is properly implemented

The codebase shows a complete implementation for sending AI-generated insights as PR comments:

  • The Console.AI.Evidence protocol for StackState generates insights about terraform plans
  • The post_comment function in lib/console/deployments/stacks.ex handles posting these insights as PR comments through the SCM-specific implementations
  • The PR comment system supports multiple SCM providers (GitHub, GitLab, Bitbucket) through the dispatcher pattern
  • The insights include plan analysis, blast radius assessment, and safety considerations as shown in the state description template
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for PR comment integration code
rg -l "pull.*request.*comment.*insight" --type elixir

Length of output: 55


Script:

#!/bin/bash
# Let's search for PR comment or review-related code more broadly
rg -A 5 "pull.*request|pr.*comment|review.*comment" --type elixir

# Also search for modules that might handle PR interactions
fd -e ex -E "_test.ex$" -x grep -l "github.*api\|pull.*request\|pr.*comment"

# Look for any insight-related code that might be connected to PRs
rg -A 5 "insight.*comment|comment.*insight" --type elixir

Length of output: 61754


Script:

#!/bin/bash
# Let's look for code that handles insights and comments together
ast-grep --pattern 'def post_comment($$$) { $$ insight $$ }'

# Also search for any code that connects insights with PR comments
rg -A 5 "insight.*review|review.*insight" --type elixir

# Look for the implementation of the AI evidence protocol for stack states
rg -A 10 "defimpl Console.AI.Evidence, for: Console.Schema.StackState" --type elixir

Length of output: 1044


Script:

#!/bin/bash
# Let's look for the code that handles PR comments and reviews
rg -A 10 "def review|def post_comment" lib/console/deployments/

# Also check for any AI-related code that might be used in PR comments
rg -A 5 "ai.*review|review.*ai" --type elixir

# Look for the connection between stack states and PR comments
rg -A 10 "def state_description|def fetch_code" lib/console/ai/evidence/

Length of output: 5686

lib/console/schema/ai_insight.ex (2)

8-8: LGTM: Clean alias addition

The StackState alias is properly added and follows the alphabetical ordering convention.


26-30: Verify database setup for the new StackState association

The has_one associations are properly defined with consistent formatting. However, let's verify the database setup for the new StackState relationship.

✅ Verification successful

Let me search for more specific information about the migration files.


Database setup for StackState association is properly configured

The database setup for the StackState relationship is correctly implemented:

  • Migration exists adding insight_id as a foreign key to stack_states table with proper reference to ai_insights
  • The belongs_to :insight, AiInsight relationship is defined in the StackState schema
  • The foreign key uses on_delete: :nilify_all which is appropriate for this relationship
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the database migration and index for the new StackState relationship

# Check for migration file that adds insight_id to stack_states
fd -e exs -e sql . priv/repo/migrations/ -x grep -l "stack_states.*insight_id"

# Check for proper index definition
fd -e exs -e sql . priv/repo/migrations/ -x grep -l "create.*index.*stack_states.*insight_id"

# Verify the StackState schema definition
rg "schema \"stack_states\"" -A 10

Length of output: 892


Script:

#!/bin/bash
# Let's look for recent migrations that might contain stack_states table creation
fd -e exs . priv/repo/migrations/ -x grep -l "create table.*stack_states"

# Also check for any references to insight_id in stack_states table
fd -e exs . priv/repo/migrations/ -x grep -A 5 -B 5 "table.*stack_states"

# Let's also verify if there are any other foreign key references in the StackState schema
rg "belongs_to|references" lib/console/schema/stack_state.ex

Length of output: 926

lib/console/ai/evidence/stack_run.ex (2)

56-56: LGTM! Good improvement in code flexibility.

The change from hardcoded "terraform" to dynamic #{run.type} makes the code more generic and maintainable, allowing it to handle different types of runs consistently.


56-56: Verify supported run types.

Since we're making the code more generic to handle different run types, let's verify all supported types to ensure proper handling throughout the codebase.

✅ Verification successful

Let me try a more focused search to find the run types.


Let me try one more focused search to find the run types in the codebase.


Let me try one more search to find the run types in the schema definitions.


Let me try one final search to find the stack run types.


Based on the search results, I can now provide a definitive response about the supported run types.

Verified supported run types: terraform, ansible, and custom

The codebase shows that Stack.Type is defined as an enum in lib/console/schema/stack.ex with three possible values:

  • terraform (0)
  • ansible (1)
  • custom (2)

This matches the usage in stack_runs schema where field :type is defined as Stack.Type, confirming these are the only supported run types throughout the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all possible run types and their handling

# Search for type definitions and pattern matches on run.type
ast-grep --pattern 'type: $_'

# Search for specific handling of run types
rg -A 2 'run.type'

# Look for type-specific handling in pattern matches
rg 'case.*type.*do'

Length of output: 101834


Script:

#!/bin/bash
# Search for run types in Elixir files
rg -t elixir "run\.type" -B2 -A2

# Search for type definitions or case statements with run types
rg -t elixir "type: ['\"].*['\"]" -B1 -A1

# Look for run type pattern matches
ast-grep --pattern 'type: $_'

Length of output: 100204


Script:

#!/bin/bash
# Search for run types in Elixir files, focusing on schema definitions and enums
rg -t elixir "type: :" -B2 -A2

# Search for run type definitions in schema files
rg -t elixir "@type.*run" -B2 -A2

# Look for run type usage in pattern matches
rg -t elixir "case.*run\.type" -B2 -A2

Length of output: 107065


Script:

#!/bin/bash
# Search for run type definitions in schema files
rg -t elixir "^  @type.*run" -B2 -A2

# Search for run type enum definitions
rg -t elixir "@type.*run.*::" -B2 -A2

# Search for run type in schema files
rg -t elixir "schema.*run" -B2 -A2

Length of output: 3630


Script:

#!/bin/bash
# Search for Stack.Type enum definition
rg -t elixir "defenum.*Type" -B2 -A2

# Search for stack run type usage in schema
rg -t elixir "field :type.*Stack\.Type" -B2 -A2

# Search for stack type in migrations
rg "add :type" migrations

Length of output: 4214

test/console/ai/pubsub/consumer_test.exs (1)

89-91: Good choice using async: false

Setting async: false is appropriate here since we're testing OpenAI integration and database operations that could have side effects.

lib/console/graphql/ai.ex (2)

97-97: LGTM! Formatting improvement.

The reformatting enhances code readability by maintaining consistent alignment with other fields.


95-97: Verify the stack_state relationship configuration.

Let's ensure the relationship between stack_state and ai_insight is properly defined in both directions.

✅ Verification successful

Relationship between stack_state and ai_insight is properly configured

The verification shows that the relationship is correctly defined in both directions:

  • In lib/console/schema/stack_state.ex: belongs_to :insight, AiInsight
  • In lib/console/schema/ai_insight.ex: has_one :stack_state, StackState, foreign_key: :insight_id

The GraphQL type definition in lib/console/graphql/deployments/stack.ex also correctly exposes this relationship through the insight field on the stack_state type.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify stack_state type definition and relationship configuration

# Check for stack_state type definition
echo "Checking for stack_state type definition..."
rg -A 10 "object :stack_state" --type elixir

# Check for belongs_to relationship in stack_state schema
echo "Checking for belongs_to relationship..."
rg "belongs_to.*:ai_insight" --type elixir

# Check for migration adding insight_id
echo "Checking for migration adding insight_id..."
fd -e exs -x rg "add.*insight_id.*stack_states" {}

Length of output: 4096


Script:

#!/bin/bash
# Let's check for the schema definition to verify the relationship
echo "Checking for stack_state schema definition..."
rg "schema.*StackState" --type elixir -A 10

# Check for any references to insight_id in stack_states
echo "Checking for insight_id references..."
rg "insight_id.*stack_states|stack_states.*insight_id" --type elixir

# Check for any recent migrations related to stack_states
echo "Checking recent migrations..."
fd ".*_stack_states.exs" --type f

Length of output: 407


Script:

#!/bin/bash
# Let's try a broader search for the schema definition
echo "Checking for StackState module..."
rg "defmodule.*StackState" --type elixir -A 10

# Look for any schema definitions related to stack_states
echo "Checking for stack_states schema..."
rg "schema \"stack_states\"" --type elixir -A 10

# Check for any belongs_to or has_one relationships between stack_states and ai_insights
echo "Checking for relationships..."
rg "belongs_to :insight|has_one :stack_state" --type elixir

# Look for migrations with a broader pattern
echo "Checking migrations with broader pattern..."
fd -e exs -x rg "create table.*stack_states|alter table.*stack_states" {}

Length of output: 2944

lib/console/graphql/deployments/stack.ex (1)

335-338: LGTM! The insight field addition enhances Terraform plan interpretability.

The implementation follows GraphQL best practices with proper use of dataloader and clear documentation.

Let's verify the consistency of insight field implementation across related objects:

✅ Verification successful

Consistent implementation of insight field confirmed across schema objects

The insight field is consistently implemented across all relevant objects:

  • stack, stack_run, stack_state
  • service, service_component
  • cluster

All implementations follow the same pattern:

  • Type: :ai_insight
  • Resolver: dataloader(Deployments)
  • Description: contextual explanation for each object type
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent implementation of insight field across objects
# Test: Search for insight field definitions in GraphQL schema
# Expect: Consistent implementation pattern (type, resolver, description)

rg --type elixir 'field :insight.*:ai_insight.*dataloader\(Deployments\)' -A 2

Length of output: 2599

go/client/models_gen.go (2)

Line range hint 1-1: Note: This is generated code

This file is automatically generated by gqlgen. Manual modifications should be avoided.


152-152: LGTM: New field addition looks correct

The addition of StackState *StackState field to the AiInsight struct is properly typed and follows the existing field pattern with the json:"stackState,omitempty" tag.

Comment on lines 33 to 43
case Console.Repo.preload(run, [:state]) do
%StackRun{state: %StackState{} = state} -> {:ok, state}
_ -> :ok
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for database operations

The current implementation doesn't handle potential database errors explicitly, which could lead to unexpected behavior.

Consider adding explicit error handling:

-    case Console.Repo.preload(run, [:state]) do
-      %StackRun{state: %StackState{} = state} -> {:ok, state}
-      _ -> :ok
+    try do
+      case Console.Repo.preload(run, [:state]) do
+        %StackRun{state: %StackState{} = state} -> {:ok, state}
+        _ -> :ok
+      end
+    rescue
+      e in [Ecto.QueryError] ->
+        Logger.error("Failed to preload stack state: #{Exception.message(e)}")
+        :ok
+    end

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +8 to +10
def generate(%StackState{run: %StackRun{} = run} = state),
do: {:ok, [state_description(state) | fetch_code(run)]}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for missing run data.

The pattern matching %StackState{run: %StackRun{} = run} will fail if run is nil. Consider adding a fallback clause to handle this case gracefully.

+def generate(%StackState{run: nil}), do: {:error, "No run data available"}
 def generate(%StackState{run: %StackRun{} = run} = state),
   do: {:ok, [state_description(state) | fetch_code(run)]}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def generate(%StackState{run: %StackRun{} = run} = state),
do: {:ok, [state_description(state) | fetch_code(run)]}
def generate(%StackState{run: nil}), do: {:error, "No run data available"}
def generate(%StackState{run: %StackRun{} = run} = state),
do: {:ok, [state_description(state) | fetch_code(run)]}

Comment on lines +33 to +40
defp fetch_code(%StackRun{} = run) do
with {:ok, f} <- Stacks.tarstream(run),
{:ok, msgs} <- code_prompt(f, run.git.folder, "I'll also include the relevant #{run.type} code below, listed in the format #{file_fmt()}") do
msgs
else
_ -> []
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and add documentation.

The current implementation silently fails by returning an empty list for any error. Consider:

  1. Adding logging for errors
  2. Documenting the expected format of the response
  3. Being more specific about which errors to catch
+  @doc """
+  Fetches relevant code from the stack run.
+  
+  Returns a list of code snippets in the format specified by file_fmt/0.
+  In case of errors, returns an empty list.
+  """
   defp fetch_code(%StackRun{} = run) do
     with {:ok, f} <- Stacks.tarstream(run),
          {:ok, msgs} <- code_prompt(f, run.git.folder, "I'll also include the relevant #{run.type} code below, listed in the format #{file_fmt()}") do
       msgs
     else
-      _ -> []
+      {:error, reason} = error ->
+        Console.Logger.warn("Failed to fetch code: #{inspect(error)}")
+        []
     end
   end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
defp fetch_code(%StackRun{} = run) do
with {:ok, f} <- Stacks.tarstream(run),
{:ok, msgs} <- code_prompt(f, run.git.folder, "I'll also include the relevant #{run.type} code below, listed in the format #{file_fmt()}") do
msgs
else
_ -> []
end
end
@doc """
Fetches relevant code from the stack run.
Returns a list of code snippets in the format specified by file_fmt/0.
In case of errors, returns an empty list.
"""
defp fetch_code(%StackRun{} = run) do
with {:ok, f} <- Stacks.tarstream(run),
{:ok, msgs} <- code_prompt(f, run.git.folder, "I'll also include the relevant #{run.type} code below, listed in the format #{file_fmt()}") do
msgs
else
{:error, reason} = error ->
Console.Logger.warn("Failed to fetch code: #{inspect(error)}")
[]
end
end

@michaeljguarino michaeljguarino force-pushed the mjg/prod-2912-terraform-plan-insights branch 6 times, most recently from 93c59e4 to d51260e Compare November 22, 2024 04:44
These are often hard to interpret too so an insight could be useful to have, will also see if i can send them w/ the pr comments
@michaeljguarino michaeljguarino force-pushed the mjg/prod-2912-terraform-plan-insights branch from d51260e to a6161e7 Compare November 22, 2024 05:45
Copy link

linear bot commented Nov 22, 2024

@michaeljguarino michaeljguarino force-pushed the mjg/prod-2912-terraform-plan-insights branch from d32c48b to b74d740 Compare November 22, 2024 15:16
@michaeljguarino michaeljguarino merged commit ce0364b into master Nov 22, 2024
15 checks passed
@michaeljguarino michaeljguarino deleted the mjg/prod-2912-terraform-plan-insights branch November 22, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants