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

fix: Init system resource #5

Merged
merged 4 commits into from
Jan 30, 2025
Merged

fix: Init system resource #5

merged 4 commits into from
Jan 30, 2025

Conversation

adityachoudhari26
Copy link
Contributor

@adityachoudhari26 adityachoudhari26 commented Jan 23, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a ctrlplane_system resource for managing systems.
    • Implemented OpenAPI code generation for client interactions.
    • Added support for workspace configuration in the provider.
  • Dependency Updates

    • Updated Go module dependencies, including the addition of OpenAPI runtime and testing libraries.
    • Removed scaffolding-related dependencies.
  • Configuration Changes

    • Updated provider configuration to use a new base URL.
    • Modified Terraform provider registry address.

Copy link

coderabbitai bot commented Jan 23, 2025

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 golangci-lint (1.62.2)

level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar."

Walkthrough

The pull request introduces significant changes to the Terraform provider for CtrlPlane. Modifications include the addition of a command for generating client code using OpenAPI specifications, restructuring the provider configuration to replace the scaffolding provider, and implementing a new system resource. Additionally, there are substantial updates to the dependencies in the go.mod file, enhancements to the provider's core functionality, and updates to the handling of workspace and base URLs.

Changes

File Change Summary
client/openapi.generate.sh Added command to generate client code using oapi-codegen from remote OpenAPI specification
examples/provider/provider.tf Replaced scaffolding provider with new CtrlPlane provider configuration and added ctrlplane provider block
go.mod Significant dependency updates, removing scaffolding and OpenAPI-related dependencies, adding new runtime and testing libraries
internal/provider/provider.go Modified base URL handling, added Workspace field, changed resource method to return NewSystemResource
internal/provider/system_resource.go Implemented new system resource with full CRUD operations and error handling
main.go Updated provider registry address
internal/provider/data_source_model.go Added DataSourceModel struct for managing data sources
client/client.go Removed a blank line (no functional changes)

Sequence Diagram

sequenceDiagram
    participant Terraform
    participant Provider
    participant CtrlPlane API
    
    Terraform->>Provider: Initialize Provider
    Provider->>CtrlPlane API: Configure Base URL
    Terraform->>Provider: Create System Resource
    Provider->>CtrlPlane API: Create System
    CtrlPlane API-->>Provider: Return System Details
    Provider-->>Terraform: Confirm Resource Creation
Loading

Poem

🐰 A Terraform Rabbit's Delight
From scaffolding's frame, we take flight,
CtrlPlane's provider, shining bright!
Systems dance with code so neat,
Our API calls now complete.
A provider's journey, pure and light! 🚀

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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

@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: 7

🔭 Outside diff range comments (2)
internal/provider/provider.go (1)

Line range hint 1-5: Update the file headers to reflect the correct project information.

The current file headers reference HashiCorp and the MPL-2.0 license, which may not be appropriate for this project. Please update the headers to align with CodeRabbit Inc.'s licensing and copyright information.

go.mod (1)

Line range hint 3-3: Fix invalid Go version.

The specified Go version 1.22.7 is invalid as it doesn't exist. The latest stable version is 1.22.0.

Apply this diff:

-go 1.22.7
+go 1.22
🧹 Nitpick comments (3)
internal/provider/provider.go (1)

94-98: Improve BaseURL handling for robustness.

The current logic for manipulating the BaseURL may not handle all edge cases, such as URLs with additional path segments or inconsistent trailing slashes. Consider using Go's net/url package to parse and construct the URL more robustly.

Example of using net/url:

import (
	"net/url"
	"path"
	// Other imports...
)

// ...

server := data.BaseURL.ValueString()
parsedURL, err := url.Parse(server)
if err != nil {
	resp.Diagnostics.AddError("Invalid Base URL", fmt.Sprintf("Failed to parse Base URL: %s", err))
	return
}

// Ensure the path ends with "/api"
parsedURL.Path = path.Join(parsedURL.Path, "/api")
server = parsedURL.String()
examples/provider/provider.tf (2)

9-11: Make base_url configurable via variables.

The hardcoded base_url should be configurable to support different environments.

Apply this diff:

+variable "ctrlplane_base_url" {
+  description = "Base URL for the CtrlPlane API"
+  type        = string
+  default     = "http://localhost:3000"
+}
+
 provider "ctrlplane" {
-  base_url = "http://localhost:3000"
+  base_url = var.ctrlplane_base_url
 }

13-16: Document workspace_id and make it configurable.

The hardcoded workspace_id should be documented and made configurable.

Apply this diff:

+variable "workspace_id" {
+  description = "CtrlPlane workspace ID (UUID v4 format)"
+  type        = string
+  default     = "5316df47-1f1c-4a5e-85e6-645e6b821616"
+
+  validation {
+    condition     = can(regex("^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$", var.workspace_id))
+    error_message = "workspace_id must be a valid UUID v4"
+  }
+}
+
 resource "ctrlplane_system" "example" {
   name         = "tf_test_official"
   slug         = "tf_test_official"
-  workspace_id = "5316df47-1f1c-4a5e-85e6-645e6b821616"
+  workspace_id = var.workspace_id
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9d8b10 and f6b493f.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • client/openapi.generate.sh (1 hunks)
  • examples/provider/provider.tf (1 hunks)
  • go.mod (4 hunks)
  • internal/provider/provider.go (5 hunks)
  • internal/provider/system_resource.go (1 hunks)
  • main.go (2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
client/openapi.generate.sh

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

🪛 GitHub Actions: Tests
client/openapi.generate.sh

[warning] File is missing required headers

main.go

[warning] File is missing required headers

internal/provider/system_resource.go

[warning] File is missing required headers

🔇 Additional comments (1)
go.mod (1)

10-10: Verify compatibility with oapi-codegen/runtime.

The addition of github.com/oapi-codegen/runtime v1.1.1 looks good, but let's verify its compatibility with the codebase.

Run the following script to check for any compatibility issues:

✅ Verification successful

Runtime package version is compatible

The code is generated using oapi-codegen v2.4.1 which is fully compatible with runtime v1.1.1. The generated code shows correct usage of runtime functions for parameter handling and JSON operations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for compatibility issues with oapi-codegen/runtime

# Test: Check if the package is imported correctly
rg -l 'github.com/oapi-codegen/runtime'

# Test: Check for any type mismatches or compilation errors
go vet ./...

Length of output: 182


Script:

#!/bin/bash
# Check how the runtime package is used and look for generator version
echo "=== Content of client/client.gen.go ==="
head -n 20 client/client.gen.go

echo -e "\n=== Runtime package usage ==="
rg "runtime\." client/client.gen.go

echo -e "\n=== Looking for oapi-codegen version or config ==="
fd "oapi-codegen" --type f

Length of output: 3937

@@ -0,0 +1,263 @@
package provider
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add the missing file headers to comply with project requirements.

The pipeline has issued a warning that the file is missing required headers. Please add the appropriate headers at the beginning of the file to adhere to the project's coding standards.

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

Comment on lines 127 to 135
plan.Id = types.StringValue(result.System.Id.String())
plan.Name = types.StringValue(result.System.Name)
plan.Slug = types.StringValue(result.System.Slug)
description := getDescription(result.System.Description)
if description != nil {
plan.Description = types.StringValue(*description)
}
plan.WorkspaceId = types.StringValue(result.System.WorkspaceId.String())

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor repeated code for setting the resource model from the API response.

The code segments in the Create, Read, and Update methods for updating the resource model from the API response are nearly identical. To improve maintainability and reduce code duplication, consider refactoring this logic into a helper function.

Example of how to refactor:

func (r *systemResource) setResourceModelFromResponse(ctx context.Context, system client.System, model *systemResourceModel) {
	model.Id = types.StringValue(system.Id.String())
	model.Name = types.StringValue(system.Name)
	model.Slug = types.StringValue(system.Slug)
	description := getDescription(system.Description)
	if description != nil {
		model.Description = types.StringValue(*description)
	} else {
		model.Description = types.StringNull()
	}
	model.WorkspaceId = types.StringValue(system.WorkspaceId.String())
}

Then, in each method, you can call this helper function:

// In Create method
r.setResourceModelFromResponse(ctx, result.System, &plan)

// In Read method
r.setResourceModelFromResponse(ctx, result.System, &state)

// In Update method
r.setResourceModelFromResponse(ctx, result.System, &plan)

Also applies to: 167-175, 228-236

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

Comment on lines 152 to 156
clientResp, err := r.client.GetSystem(ctx, state.Id.ValueString())
if err != nil {
resp.Diagnostics.AddError("Failed to read system", fmt.Sprintf("Failed to read system: %s", err))
return
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle missing resources gracefully in the Read method.

If the system resource no longer exists (e.g., the API returns a 404 Not Found error), the Read method should remove the resource from the state without returning an error. This allows Terraform to manage the resource lifecycle correctly when resources are deleted outside of Terraform.

Consider modifying the error handling as follows:

if err != nil {
	if clientResp != nil && clientResp.StatusCode == http.StatusNotFound {
		// Resource not found, remove from state
		resp.State.RemoveResource(ctx)
		return
	}
	resp.Diagnostics.AddError("Failed to read system", fmt.Sprintf("Failed to read system: %s", err))
	return
}
🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

Comment on lines 253 to 262
clientResp, err := r.client.DeleteSystem(ctx, state.Id.ValueString())
if err != nil {
resp.Diagnostics.AddError("Failed to delete system", fmt.Sprintf("Failed to delete system: %s", err))
return
}

if clientResp.StatusCode != http.StatusOK {
resp.Diagnostics.AddError("Failed to delete system", fmt.Sprintf("Failed to delete system: %s", clientResp.Status))
return
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle missing resources gracefully in the Delete method.

Similar to the Read method, if the system resource is already deleted (e.g., the API returns a 404 Not Found error), the Delete method should succeed without returning an error. This ensures that Terraform can remove the resource from the state and does not treat the operation as a failure.

Consider updating the error handling:

if err != nil {
	if clientResp != nil && clientResp.StatusCode == http.StatusNotFound {
		// Resource already deleted
		return
	}
	resp.Diagnostics.AddError("Failed to delete system", fmt.Sprintf("Failed to delete system: %s", err))
	return
}
🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

@@ -1,18 +1,15 @@

package main
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add the missing file headers to comply with project requirements.

The pipeline has issued a warning that the file is missing required headers. Please include the necessary headers at the top of the file to meet the project's coding standards.

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

@@ -0,0 +1 @@
oapi-codegen -config openapi.client.yaml https://raw.githubusercontent.com/ctrlplanedev/ctrlplane/refs/heads/main/openapi.v1.json
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add required shell script headers and error handling.

The script is missing essential components:

  1. Shebang line to specify the shell interpreter
  2. Required headers (based on pipeline failure)
  3. Error handling for potential failures

Apply this diff to improve the script:

+#!/bin/bash
+
+# Copyright (c) 2024 CtrlPlane
+# SPDX-License-Identifier: Apache-2.0
+
+set -euo pipefail
+
+# Verify oapi-codegen is installed
+if ! command -v oapi-codegen &> /dev/null; then
+    echo "Error: oapi-codegen is not installed"
+    exit 1
+fi
+
+# Download and verify OpenAPI spec
+SPEC_URL="https://raw.githubusercontent.com/ctrlplanedev/ctrlplane/refs/heads/main/openapi.v1.json"
+TEMP_SPEC=$(mktemp)
+
+if ! curl -sSfL "$SPEC_URL" -o "$TEMP_SPEC"; then
+    echo "Error: Failed to download OpenAPI spec"
+    rm -f "$TEMP_SPEC"
+    exit 1
+fi
+
+if ! jq empty "$TEMP_SPEC" 2>/dev/null; then
+    echo "Error: Invalid JSON in OpenAPI spec"
+    rm -f "$TEMP_SPEC"
+    exit 1
+fi
+
 oapi-codegen -config openapi.client.yaml https://raw.githubusercontent.com/ctrlplanedev/ctrlplane/refs/heads/main/openapi.v1.json
+rm -f "$TEMP_SPEC"
📝 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
oapi-codegen -config openapi.client.yaml https://raw.githubusercontent.com/ctrlplanedev/ctrlplane/refs/heads/main/openapi.v1.json
#!/bin/bash
# Copyright (c) 2024 CtrlPlane
# SPDX-License-Identifier: Apache-2.0
set -euo pipefail
# Verify oapi-codegen is installed
if ! command -v oapi-codegen &> /dev/null; then
echo "Error: oapi-codegen is not installed"
exit 1
fi
# Download and verify OpenAPI spec
SPEC_URL="https://raw.githubusercontent.com/ctrlplanedev/ctrlplane/refs/heads/main/openapi.v1.json"
TEMP_SPEC=$(mktemp)
if ! curl -sSfL "$SPEC_URL" -o "$TEMP_SPEC"; then
echo "Error: Failed to download OpenAPI spec"
rm -f "$TEMP_SPEC"
exit 1
fi
if ! jq empty "$TEMP_SPEC" 2>/dev/null; then
echo "Error: Invalid JSON in OpenAPI spec"
rm -f "$TEMP_SPEC"
exit 1
fi
oapi-codegen -config openapi.client.yaml https://raw.githubusercontent.com/ctrlplanedev/ctrlplane/refs/heads/main/openapi.v1.json
rm -f "$TEMP_SPEC"
🧰 Tools
🪛 Shellcheck (0.10.0)

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

🪛 GitHub Actions: Tests

[warning] File is missing required headers

Comment on lines +1 to +7
terraform {
required_providers {
ctrlplane = {
source = "ctrlplane/ctrlplane"
}
}
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add version constraints for required provider.

The required_providers block should specify version constraints to ensure reproducible infrastructure.

Apply this diff:

 terraform {
   required_providers {
     ctrlplane = {
       source = "ctrlplane/ctrlplane"
+      version = "~> 1.0"
     }
   }
 }
📝 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
terraform {
required_providers {
ctrlplane = {
source = "ctrlplane/ctrlplane"
}
}
}
terraform {
required_providers {
ctrlplane = {
source = "ctrlplane/ctrlplane"
version = "~> 1.0"
}
}
}

Copy link

@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: 1

🧹 Nitpick comments (2)
internal/provider/system_resource.go (1)

131-131: Remove leftover debug statement.

Using fmt.Println("system from the api", *system.JSON201) might be useful for local debugging, but it should be removed in production code to keep logs clean.

-    fmt.Println("system from the api", *system.JSON201)
🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

internal/provider/provider.go (1)

146-150: Use path join or handle trailing slashes more robustly.

Building the final server string with TrimSuffix works but can be fragile if new variants appear. You could consider path.Join or carefully handle edge cases for appended slashes.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6b493f and d76cf13.

📒 Files selected for processing (4)
  • examples/provider/provider.tf (1 hunks)
  • internal/provider/data_source_model.go (1 hunks)
  • internal/provider/provider.go (6 hunks)
  • internal/provider/system_resource.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • examples/provider/provider.tf
🧰 Additional context used
🪛 GitHub Actions: Tests
internal/provider/data_source_model.go

[warning] File is missing required headers

internal/provider/system_resource.go

[warning] File is missing required headers

🔇 Additional comments (11)
internal/provider/system_resource.go (4)

1-1: Add the missing file headers to comply with project requirements.

This file is missing the required file headers as indicated by the pipeline.

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers


167-175: Refactor repeated code for setting the resource model from the API response.

The logic in the Read method looks very similar to the helper function setSystemResourceData(). Consider calling that function here to reduce code duplication.

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers


143-180: Handle missing resources gracefully in the Read method.

If the resource no longer exists (e.g. the API returns 404), remove it from the state without returning an error.

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers


247-256: Handle missing resources gracefully in the Delete method.

Similar to the Read method, if the resource is already deleted or not found, gracefully handle the situation by removing the resource from the state without reporting an error.

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

internal/provider/provider.go (7)

8-15: New imports look good.

No immediate concerns with importing these packages (errors, strings, github.com/google/uuid).


40-40: Verify usage of optional workspace.

Workspace is marked as optional, yet the code in getWorkspace returns an error if the workspace string is empty. Please confirm if making this optional is intentional or if it should be mandatory.


59-62: Consider making the workspace attribute mandatory.

Given that an empty workspace triggers an error in getWorkspace, it might be more consistent to require this attribute in the provider schema.


74-117: Workspace lookup functions look correct.

The logic to retrieve a workspace by ID or slug, and handle 404 scenarios, appears clear and well-structured.


128-128: Environment variable fallback for BaseURL is fine.

The fallback logic ensures users can set the CTRLPLANE_BASE_URL environment variable when the provider config omits the base_url.


160-170: Workspace retrieval logic is sound.

The code correctly extracts the workspace from user config, calls getWorkspace, and handles potential errors.


178-178: Returning NewSystemResource is consistent with the new resource introduction.

No issues observed.

@@ -0,0 +1,12 @@
package provider
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add the required file headers.

The pipeline indicates this file is missing required file headers. Please add the necessary headers at the top of the file to comply with project standards and pass the checks.

🧰 Tools
🪛 GitHub Actions: Tests

[warning] File is missing required headers

Copy link

@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: 2

🧹 Nitpick comments (4)
internal/provider/provider.go (4)

59-62: Enhance workspace field documentation.

Consider providing more detailed documentation for the workspace field to help users understand:

  • The expected format (UUID or slug)
  • Default behavior if not specified
  • Examples of valid values
 "workspace": schema.StringAttribute{
-    MarkdownDescription: "The workspace to use",
+    MarkdownDescription: "The workspace to use for all operations. Can be specified as either a UUID or a slug. Required for all operations.",
     Optional:            true,
 },

91-106: Optimize workspace retrieval by slug.

The getWorkspaceBySlug function makes an unnecessary additional API call by calling getWorkspaceById after successfully retrieving the workspace.

 func getWorkspaceBySlug(ctx context.Context, slug string, client *client.ClientWithResponses) (uuid.UUID, error) {
     validatedWorkspace, err := client.GetWorkspaceBySlugWithResponse(ctx, slug)
     if err != nil {
         return uuid.Nil, err
     }

     if validatedWorkspace.JSON200 != nil {
-        return getWorkspaceById(ctx, validatedWorkspace.JSON200.Id, client)
+        return validatedWorkspace.JSON200.Id, nil
     }

     if validatedWorkspace.JSON404 != nil {
         return uuid.Nil, errors.New("workspace not found")
     }

     return uuid.Nil, errors.New("failed to get workspace")
 }

176-187: Document resource changes and empty implementations.

Please add comments explaining:

  • The transition from previous resources to SystemResource
  • Why DataSources and Functions are currently empty
 func (p *CtrlplaneProvider) Resources(ctx context.Context) []func() resource.Resource {
+    // Currently only supporting system resource management
+    // Previous target resource has been deprecated
     return []func() resource.Resource{
         NewSystemResource,
     }
 }

 func (p *CtrlplaneProvider) DataSources(ctx context.Context) []func() datasource.DataSource {
+    // Data sources will be implemented in future versions
     return []func() datasource.DataSource{}
 }

 func (p *CtrlplaneProvider) Functions(ctx context.Context) []func() function.Function {
+    // Custom functions will be implemented in future versions
     return []func() function.Function{}
 }

84-86: Enhance error messages with more context.

The error messages could be more specific and actionable.

 if validatedWorkspace.JSON404 != nil {
-    return uuid.Nil, errors.New("workspace not found")
+    return uuid.Nil, fmt.Errorf("workspace with ID %s not found - please verify the workspace exists", workspaceID)
 }

 if validatedWorkspace.JSON404 != nil {
-    return uuid.Nil, errors.New("workspace not found")
+    return uuid.Nil, fmt.Errorf("workspace with slug %s not found - please verify the workspace exists", slug)
 }

 if workspace == "" {
-    return uuid.Nil, errors.New("workspace is required")
+    return uuid.Nil, errors.New("workspace is required - specify either a workspace UUID or slug in the provider configuration")
 }

Also applies to: 101-103, 109-111

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0549737 and 335cb41.

📒 Files selected for processing (3)
  • examples/provider/provider.tf (1 hunks)
  • internal/provider/provider.go (6 hunks)
  • internal/provider/system_resource.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • examples/provider/provider.tf
  • internal/provider/system_resource.go

Comment on lines +74 to +89
func getWorkspaceById(ctx context.Context, workspaceID uuid.UUID, client *client.ClientWithResponses) (uuid.UUID, error) {
validatedWorkspace, err := client.GetWorkspaceWithResponse(ctx, workspaceID)
if err != nil {
return uuid.Nil, err
}

if validatedWorkspace.JSON200 != nil {
return validatedWorkspace.JSON200.Id, nil
}

if validatedWorkspace.JSON404 != nil {
return uuid.Nil, errors.New("workspace not found")
}

return uuid.Nil, errors.New("failed to get workspace")
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add context timeout handling for API calls.

The workspace retrieval functions should handle context timeouts to ensure they don't hang indefinitely on slow API responses.

+import "time"

 func getWorkspaceById(ctx context.Context, workspaceID uuid.UUID, client *client.ClientWithResponses) (uuid.UUID, error) {
+    ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+    defer cancel()
     validatedWorkspace, err := client.GetWorkspaceWithResponse(ctx, workspaceID)
     // ... rest of the function
 }

 func getWorkspaceBySlug(ctx context.Context, slug string, client *client.ClientWithResponses) (uuid.UUID, error) {
+    ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+    defer cancel()
     validatedWorkspace, err := client.GetWorkspaceBySlugWithResponse(ctx, slug)
     // ... rest of the function
 }

Also applies to: 91-117

Comment on lines +146 to +149
server := data.BaseURL.ValueString()
server = strings.TrimSuffix(server, "/")
server = strings.TrimSuffix(server, "/api")
server = server + "/api"
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve URL manipulation robustness.

The current URL manipulation logic could be made more robust by using proper URL parsing.

+import "net/url"

-    server := data.BaseURL.ValueString()
-    server = strings.TrimSuffix(server, "/")
-    server = strings.TrimSuffix(server, "/api")
-    server = server + "/api"
+    baseURL, err := url.Parse(data.BaseURL.ValueString())
+    if err != nil {
+        resp.Diagnostics.AddError("Invalid base URL", err.Error())
+        return
+    }
+    baseURL.Path = strings.TrimSuffix(baseURL.Path, "/")
+    baseURL.Path = strings.TrimSuffix(baseURL.Path, "/api")
+    baseURL.Path = baseURL.Path + "/api"
+    server := baseURL.String()
📝 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
server := data.BaseURL.ValueString()
server = strings.TrimSuffix(server, "/")
server = strings.TrimSuffix(server, "/api")
server = server + "/api"
baseURL, err := url.Parse(data.BaseURL.ValueString())
if err != nil {
resp.Diagnostics.AddError("Invalid base URL", err.Error())
return
}
baseURL.Path = strings.TrimSuffix(baseURL.Path, "/")
baseURL.Path = strings.TrimSuffix(baseURL.Path, "/api")
baseURL.Path = baseURL.Path + "/api"
server := baseURL.String()

@adityachoudhari26 adityachoudhari26 merged commit 0d74259 into main Jan 30, 2025
3 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant