feat: Add Deployment Resource #8
Conversation
WalkthroughThe changes enhance the overall project by updating test targets in the GNUmakefile, expanding the client API with new constants, types, and methods, and updating documentation for both data sources and resources. Terraform examples now illustrate the use of the new "ctrlplane" configurations. Dependency versions in go.mod were bumped, and integration tests, provider code, and multiple deployment resource files were updated to replace legacy fields with new ones. New CRUD operations for deployment resources were added along with comprehensive tests, and a new interface was introduced to improve client retrieval within the provider. Changes
Sequence Diagram(s)sequenceDiagram
participant TF as Terraform CLI
participant TP as Provider
participant DR as Deployment Resource
participant API as API Client
TF->>TP: Apply deployment configuration
TP->>DR: Invoke Create/Update/Read/Delete method
DR->>API: Send API request (with payload, parameters)
API-->>DR: Return response (success/error)
DR->>TP: Update state with API response data
TP-->>TF: Complete operation with new state
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
client/client.gen.go (1)
108-108: Consider using time.Duration for timeouts
Using a pointer to int for timeouts can be less descriptive. Switch to time.Duration for clarity.-Timeout *int `json:"timeout"` +Timeout *time.Duration `json:"timeout"`internal/provider/environment_data_source.go (1)
175-175: Initialize new field to prevent null state issues.Setting
data.DeploymentVersionChannelsto an empty slice avoids null references. Verify if API data should populate it in the future.internal/resources/deployment/read.go (2)
28-34: Clarify error messaging for null deployment ID.Returning "This is a bug in the provider" might be confusing to users who encounter this scenario. Consider rewording the error message to help them diagnose and correct possible configuration issues.
63-69: Handle non-200 status codes more distinctly.Right now, 404 is handled separately, but the rest of the non-200 statuses are lumped together. You could provide more granular error messaging or actions for 4xx vs. 5xx responses, which might accelerate debugging and clarify failure modes.
internal/resources/deployment/data_source_test.go (1)
26-30: Consider adding a destroy check.After creating a deployment (step 1), you might include a step to destroy it to confirm that the data source also fails or becomes empty when referencing a removed resource. This ensures more robust validation and prevents stale state.
internal/resources/deployment/update_test.go (1)
38-44: Expand testing to non-mandatory fields.Consider adding extra steps that modify optional fields or partial updates (if applicable) to increase confidence that all field changes are handled properly.
examples/resources/ctrlplane_deployment/resource.tf (1)
12-16: Resource definition for ctrlplane_system is valid.Ensure the slug is unique in your environment to avoid naming conflicts.
internal/resources/deployment/create.go (1)
30-36: Consider pinpointing which fields are missing.Here, a single error is raised if any of the required fields (
name,system_id,slug, orjob_agent_config) is missing. It could be more user-friendly to specify which particular fields are missing, rather than grouping them under a single error message.examples/resources/ctrlplane_deployment/datadsource.tf (2)
11-14: Avoid hard-coded deployment ID for flexibility.Using a hard-coded ID here can hinder reusability across environments. Consider exposing this as a variable or environment-based configuration so that the example remains portable and user-friendly.
25-43: Consider parameterizing resource creation.While this is an example, providing optional variables for the
name,slug, anddescriptionwould enhance versatility and showcase a more dynamic configuration approach.internal/resources/deployment/update.go (1)
67-75: Revisit the commented-out resource filter logic.The code is commented-out, suggesting future or partial implementation. Confirm whether it's needed for updates. Removing or implementing it avoids confusion.
internal/resources/deployment/read_test.go (2)
36-48: Recheck repeated configuration usage.
This second test step re-applies the same configuration, which can be useful to verify idempotence. However, consider adding a slight variation (e.g., updated description) between steps to confirm that changes are being properly tracked and applied.
53-76: Validate negative cases.
The functiontestAccDeploymentConfigBasicreliably configures a system and a deployment. Although the happy path is tested, consider adding negative or edge case tests (e.g., invalidslugor missing required attributes) to increase confidence in error handling.internal/resources/deployment/resource.go (1)
98-102: Reassess the requirement for job_agent_config.
job_agent_configis marked asRequired. If a deployment legitimately does not need config in certain scenarios, consider making it optional or conditionally required whenjob_agent_idis specified.internal/resources/deployment/data_source.go (1)
146-178: Consider Handling theResourceFilter.
model.ResourceFilteris always set to a null map at line 175. If this field is not implemented or needed, consider removing it for clarity. If you plan to implement it in the future, a corresponding TODO comment can help clarify its intended usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
GNUmakefile(3 hunks)client/client.gen.go(35 hunks)docs/data-sources/deployment.md(1 hunks)docs/data-sources/environment.md(1 hunks)docs/resources/deployment.md(1 hunks)docs/resources/environment.md(1 hunks)examples/resources/ctrlplane_deployment/datadsource.tf(1 hunks)examples/resources/ctrlplane_deployment/resource.tf(1 hunks)go.mod(2 hunks)internal/integration/environment_test.go(20 hunks)internal/provider/environment_data_source.go(1 hunks)internal/provider/environment_model.go(3 hunks)internal/provider/environment_resource.go(1 hunks)internal/provider/environment_schema.go(2 hunks)internal/provider/provider.go(3 hunks)internal/resources/deployment/create.go(1 hunks)internal/resources/deployment/create_test.go(1 hunks)internal/resources/deployment/data_source.go(1 hunks)internal/resources/deployment/data_source_test.go(1 hunks)internal/resources/deployment/delete.go(1 hunks)internal/resources/deployment/delete_test.go(1 hunks)internal/resources/deployment/import_test.go(1 hunks)internal/resources/deployment/model.go(1 hunks)internal/resources/deployment/read.go(1 hunks)internal/resources/deployment/read_test.go(1 hunks)internal/resources/deployment/resource.go(1 hunks)internal/resources/deployment/schema.go(1 hunks)internal/resources/deployment/update.go(1 hunks)internal/resources/deployment/update_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
internal/resources/deployment/update_test.go (1)
internal/resources/deployment/create_test.go (1) (1)
testAccProtoV6ProviderFactories(43:45)
internal/resources/deployment/delete_test.go (1)
internal/resources/deployment/create_test.go (2) (2)
testAccProtoV6ProviderFactories(43:45)testAccDeploymentConfig(47:70)
internal/resources/deployment/create_test.go (1)
internal/provider/provider.go (1) (1)
New(255:261)
internal/resources/deployment/read_test.go (1)
internal/resources/deployment/create_test.go (1) (1)
testAccProtoV6ProviderFactories(43:45)
internal/resources/deployment/import_test.go (2)
internal/resources/deployment/create_test.go (1) (1)
testAccProtoV6ProviderFactories(43:45)internal/resources/deployment/update_test.go (1) (1)
testAccDeploymentConfigWithDesc(49:72)
internal/provider/environment_model.go (1)
internal/provider/resource_filter.go (1) (1)
ResourceFilterModel(74:81)
🔇 Additional comments (82)
go.mod (2)
35-35: Check for potential compatibility changes in go-cty v1.5.0
While likely safe, ensure code usinggo-ctyremains fully compatible.
75-75: Confirm google.golang.org/genproto update
Verify no breaking changes or new proto methods that might require updates.internal/provider/provider.go (4)
15-15: Added import for deployment package
This import seamlessly integrates the new deployment functionalities.
36-38: New ClientProvider interface
Provides a clear contract for retrieving the client within the provider.
240-240: Added deployment resource
Incorporates the new resource creation method. Looks good.
247-247: Added deployment data source
Adds a data source for retrieving deployment details. No issues found.docs/resources/environment.md (1)
25-25: Documented deployment_version_channels field
Accurately reflects the new optional field in the environment resource.internal/provider/environment_schema.go (2)
61-62: Added deployment_version_channels to resource schema
Provides an optional list of version channels. Implementation looks correct.
133-134: Added deployment_version_channels to data source schema
Allows retrieving channel data from existing environments.internal/integration/environment_test.go (11)
58-71: Consistent rename to deploymentVersionChannels
These changes correctly update the field name and pass an empty slice. No issues found.
120-133: Maintaining consistency with new field name
Good job keeping the rename consistent across environment creation.
178-197: Renaming to deploymentVersionChannels
All references align with the new field name. Looks good.
252-272: DeploymentVersionChannels usage
Continues the consistent pattern of setting an empty slice. No issues.
329-349: Rename continues for environment with simple filter
Change appears correct.
402-434: Consistent usage for environment with complex filter
The updated field usage is correct.
503-542: Date condition filter environment
The assignment of deploymentVersionChannels is properly renamed. Looks fine.
653-669: Multi-environment scenario
Field rename reflects the new model. Approved.
700-746: Nested comparison conditions
Change is aligned; no further concerns.
803-843: Mixed condition types
Everything is consistent with the rename.
909-966: Deeply nested conditions
Renamed field usage is correct.internal/provider/environment_model.go (4)
11-19: Rename ReleaseChannels → DeploymentVersionChannels
Struct field name change is consistent and correct.
23-30: Field rename in data source model
Matches the newly introduced field in the environment resource model.
40-41: Default-setting logic for environment model
Correctly initializes an empty slice if nil.
52-53: Default-setting logic for data source model
Same approach as environment model, no issues.client/client.gen.go (26)
65-70: Define Cloud Provider Region Constants
The newly added constants for AWS, Azure, and GCP look straightforward and align well with the existing pattern.
86-96: Add CloudRegionGeoData struct
Storing latitude/longitude as float32 seems acceptable. Consider whether storing the timezone as a standard Go type (e.g., time.Location) might add clarity, although a string is still valid.
105-105: Check negative retry counts
Ensure negative values for RetryCount are either disallowed or handled properly in downstream logic.
113-115: New fields in Environment
No immediate issues with CreatedAt and Description. Ensure time.Time is set accurately and consider how null descriptions are handled.
116-117: Directory field
Documentation is clear, and Directory being a string is straightforward.
283-295: Introduce UpdateDeployment struct
Fields look appropriate for deployment updates. The AdditionalProperties mechanism can add flexibility at runtime.
531-532: Alias UpdateDeploymentJSONRequestBody
This type alias simplifies request body usage. Looks fine.
573-682: Implement AdditionalProperties for UpdateDeployment
The custom JSON marshal/unmarshal logic effectively accommodates extra fields. Ensure careful usage to avoid unintentional data leaks.
910-912: Add GetCloudProviderRegions to ClientInterface
This interface method is consistent with the earlier constant definitions.
924-928: Add UpdateDeployment methods to ClientInterface
Providing both “with body” and a typed approach ensures flexibility.
1040-1044: ListDeployments and ListEnvironments in ClientInterface
New listing methods match the naming convention. No issues found.
1048-1049: ListResources in ClientInterface
Straightforward addition, consistent with other resource-based methods.
1057-1059: GetResourcesByFilter in ClientInterface
Eases retrieval of filtered resources. No immediate concerns.
1060-1062: ListSystems in ClientInterface
Useful system enumeration call. Looks properly defined.
1064-1073: Add GetCloudProviderRegions client method
Implementation follows typical pattern for GET requests. No issues found.
1124-1135: UpdateDeploymentWithBody client method
Consistent with the codebase’s approach to being flexible with request bodies.
1136-1147: UpdateDeployment client method
Marshals typed request body. Looks good and matches patterns used elsewhere.
1640-1650: *Add (c Client) ListDeployments
Method definition adheres to established naming and request patterns.
1652-1663: *Add (c Client) ListEnvironments
Well-aligned with environment operations.
1676-1687: *Add (c Client) ListResources
Method looks consistent with resource listing logic.
1712-1723: *Add (c Client) GetResourcesByFilter
The introduced filter parameter is clearly named.
1724-1735: *Add (c Client) ListSystems
Consistent naming and usage for system listing endpoint.
1736-1768: NewGetCloudProviderRegionsRequest function
Generates a standard GET request with path parameters. No issues noted.
1878-1887: NewUpdateDeploymentRequest
Creates JSON-based PATCH request for deployment updates. Implementation is clear.
1890-1923: NewUpdateDeploymentRequestWithBody
Parallel method for handling an arbitrary body. Matches standard patterns.
1925-1964: NewDeleteReleaseChannelRequest
Generates a DELETE request for named release channels. Code is consistent.docs/data-sources/environment.md (1)
25-25: New attribute aligned with schema changes.The addition of
deployment_version_channelsis consistent with the refactoring fromrelease_channels. Looks good.internal/resources/deployment/schema.go (1)
1-59: Schema definition appears well-structured.This new file cleanly outlines essential attributes for the deployment data source. No immediate issues found.
docs/data-sources/deployment.md (1)
1-33: Documentation is thorough and consistent.The data source attributes and descriptions accurately reflect the schema. Looks good.
GNUmakefile (3)
42-42: Including ./internal/resources/... path for acceptance testsExtending the acceptance test path to cover deployment resources is a good move for comprehensive test coverage.
61-61: Matching ./internal/resources/... path for testacc-quietMirroring the same change for the quiet mode ensures uniform coverage and consistency.
92-92: Adding testacc-quiet to .PHONYDeclaring this target as phony is a proper approach for clarity and correctness in the makefile.
docs/resources/deployment.md (1)
1-78: Comprehensive resource documentationThe documentation for
ctrlplane_deploymentis clear, with a solid example usage and schema explanation. This significantly aids discoverability and usability.internal/resources/deployment/import_test.go (1)
16-37: Thorough import test coverageThis test correctly verifies import functionality for deployments, including ignoring dynamic fields like job_agent_config. Implementation aligns well with existing test patterns.
internal/resources/deployment/model.go (1)
1-25: Well-defined struct for deployment modelingThe
DeploymentModelstruct and its aliases are concise, aligning well with Terraform SDK types. No issues found.internal/resources/deployment/data_source_test.go (1)
32-43: Test coverage looks good!Using
resource.TestCheckResourceAttrPairto verify attributes between the resource and data source is a solid approach. It ensures consistency and helps catch subtle differences between the two. No issues found here.internal/resources/deployment/update_test.go (1)
30-36: Great structure for validating resource updates.The test correctly verifies the resource attributes before and after the update, ensuring stable behavior over multiple steps.
examples/resources/ctrlplane_deployment/resource.tf (3)
1-7: Looks good for basic provider setup.No immediate issues with the Terraform block or provider requirements.
9-9: Provider block is straightforward.No further configuration is specified, which is fine if defaults work for your environment.
18-39: Deployment resource logic appears correct.References the system correctly, sets job_agent_config, retry_count, timeout, and resource_filter meaningfully.
internal/provider/environment_resource.go (3)
173-178: Channel collection logic is fine.Ensures only valid channel strings are appended.
181-186: Request structure update is consistent.Switching to "DeploymentVersionChannels" matches the rest of the code changes, no issues spotted.
190-195: Logging looks correct.Ensures debugging info is printed with updated field name.
internal/resources/deployment/create_test.go (3)
1-41: Acceptance test setup is clear.Generates a random name, references system resource, and validates creation. Good approach for integration testing.
43-45: Protocol v6 provider factory usage is standard.No issues, ensures the provider is available for testing.
47-70: Terraform config creation is well structured.System is created first, then the deployment references it; job_agent_config is set properly.
internal/resources/deployment/delete.go (1)
16-83: Deletion process is robust.Properly checks ID, handles non-existent resources gracefully, and logs relevant details.
internal/resources/deployment/create.go (2)
93-97: Verify the float32 conversion for retry count.The code converts the
RetryCountfromint64tofloat32. If large values are used, this approach risks losing precision. Ensure the API actually requires a floating-point type. Otherwise, consider using an integer type to avoid precision-related issues.
20-140: Overall, the creation logic is robust.The function comprehensively checks for required fields, handles optional fields gracefully, and manages errors thoroughly, aligning well with Terraform’s resource creation patterns.
examples/resources/ctrlplane_deployment/datadsource.tf (1)
1-52: File content looks solid overall.All resources, data sources, and outputs are declared clearly, illustrating a concise usage example of the provider.
internal/resources/deployment/update.go (1)
19-157: Overall update logic is structured and aligns with Terraform patterns.The error handling is thorough, and partial updates are correctly managed. Centralizing and resolving any type mismatches will further solidify this method.
internal/resources/deployment/delete_test.go (2)
36-38: Approve the resource removal approach.
Applying an empty configuration is a standard practice in Terraform acceptance tests for verifying resource deletion. This approach is clear and effective.
47-60: Consider verifying actual deletion via API.
testAccCheckDeploymentDestroycurrently confirms that the resource is removed from the state. For complete correctness, you could optionally query the backend API to ensure the resource no longer exists.internal/resources/deployment/data_source.go (5)
1-19: Looks Good!The initial package declaration, license header, and imports are appropriate. No issues found here.
24-30: No Concerns with the Constructor.
NewDataSourceproperly initializes the data source instance.
32-34: Usage ofMetadatais Correct.The data source type name is correctly formed by appending
_deploymentto the provider type name.
36-38: Schema Registration Looks Good.
GetDeploymentDataSourceSchema()is referenced properly to provide the schema.
40-68: Provider Reflection Configuration is Properly Handled.Good error handling for missing or invalid
Client, ensuring that the data source references the correct client instance.
| if !plan.Timeout.IsNull() { | ||
| timeout := int(plan.Timeout.ValueInt64()) | ||
| updateRequest.Timeout = &timeout | ||
| } |
There was a problem hiding this comment.
Maintain consistent types for Timeout as well.
In the Create method, Timeout is cast to a float32, whereas here it is cast to an int. Ensure the API’s expected type matches across create and update if they both refer to the same field.
-if !plan.Timeout.IsNull() {
- timeout := int(plan.Timeout.ValueInt64())
- updateRequest.Timeout = &timeout
+if !plan.Timeout.IsNull() {
+ timeoutInt := int(plan.Timeout.ValueInt64())
+ timeoutFloat := float32(timeoutInt)
+ updateRequest.Timeout = &timeoutFloat
}📝 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.
| if !plan.Timeout.IsNull() { | |
| timeout := int(plan.Timeout.ValueInt64()) | |
| updateRequest.Timeout = &timeout | |
| } | |
| if !plan.Timeout.IsNull() { | |
| timeoutInt := int(plan.Timeout.ValueInt64()) | |
| timeoutFloat := float32(timeoutInt) | |
| updateRequest.Timeout = &timeoutFloat | |
| } |
| if !plan.RetryCount.IsNull() { | ||
| retryCount := int(plan.RetryCount.ValueInt64()) | ||
| updateRequest.RetryCount = &retryCount | ||
| } |
There was a problem hiding this comment.
Ensure consistency with the Create method’s RetryCount type.
The Create method uses a float for RetryCount while this Update method uses an int pointer. This inconsistency can cause confusion and potential mismatches in the API if it expects a single type. Unify the approach to maintain consistency across create and update operations.
-if !plan.RetryCount.IsNull() {
- retryCount := int(plan.RetryCount.ValueInt64())
- updateRequest.RetryCount = &retryCount
+if !plan.RetryCount.IsNull() {
+ retryCountInt := int(plan.RetryCount.ValueInt64())
+ retryCountFloat := float32(retryCountInt)
+ updateRequest.RetryCount = &retryCountFloat
}📝 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.
| if !plan.RetryCount.IsNull() { | |
| retryCount := int(plan.RetryCount.ValueInt64()) | |
| updateRequest.RetryCount = &retryCount | |
| } | |
| if !plan.RetryCount.IsNull() { | |
| retryCountInt := int(plan.RetryCount.ValueInt64()) | |
| retryCountFloat := float32(retryCountInt) | |
| updateRequest.RetryCount = &retryCountFloat | |
| } |
| if config.ID.IsNull() && config.Name.IsNull() && config.SystemID.IsNull() && config.Slug.IsNull() { | ||
| resp.Diagnostics.AddError( | ||
| "Missing Required Attribute", | ||
| "At least one of id, name, system_id, or slug must be provided to find a deployment", | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Contradictory Logic in Attribute Requirements.
At lines 78–84, the code indicates one or more of “id”, “name”, “system_id”, or “slug” is required to locate a deployment. However, at lines 86–140, the logic exclusively uses the “id” field to retrieve the deployment and ignores the others. This creates a mismatch between user expectations (they can provide name/system_id/slug) and the actual lookup behavior (only “id” is used).
Please update the code to either:
- Support lookups by name, system_id, or slug, or
- Remove references to those fields if only “id” is intended.
Also applies to: 86-140
|
The structure of having a specific 'deployment' package is a more intuitive. LGTM |
Summary by CodeRabbit
ctrlplane_deploymentandctrlplane_environmentdata sources, along with updates to existing documentation.