fix: Tfe sync command #4
Conversation
|
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new synchronization feature for the CLI, focusing on Terraform workspace management. The changes involve adding a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Sync Command
participant TFClient as Terraform Client
participant CPClient as Ctrlplane API
CLI->>TFClient: Retrieve Workspaces
TFClient-->>CLI: Return Workspace List
CLI->>CPClient: Upsert Resource Provider
CPClient-->>CLI: Provider ID
CLI->>CPClient: Set Resources
CPClient-->>CLI: Synchronization Result
Poem
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? 🪧 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: 5
🧹 Nitpick comments (1)
cmd/ctrlc/root/sync/sync.go (1)
Line range hint
13-17: Update example documentation to include TFE sync command.The example documentation shows
aws-eksandgoogle-gkebut doesn't include the newly addedtfesubcommand. Please update it to include a TFE example:Example: heredoc.Doc(` $ ctrlc sync aws-eks $ ctrlc sync google-gke + $ ctrlc sync tfe --organization my-org `),
📜 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 (5)
cmd/ctrlc/root/root.go(2 hunks)cmd/ctrlc/root/sync/sync.go(2 hunks)cmd/ctrlc/root/sync/tfe/tfe-org-workspaces.go(1 hunks)cmd/ctrlc/root/sync/tfe/tfe.go(1 hunks)go.mod(2 hunks)
🔇 Additional comments (4)
cmd/ctrlc/root/root.go (1)
10-10: LGTM!The sync command is properly integrated into the root command structure.
Also applies to: 48-48
go.mod (3)
27-33: LGTM! HashiCorp dependencies are appropriate for TFE integration.The addition of HashiCorp's official
go-tfeclient and its supporting libraries is the correct approach for implementing TFE synchronization functionality.
Line range hint
26-57: Verify security advisories for new dependencies.Let's check for any known security vulnerabilities in the newly added dependencies.
✅ Verification successful
No security vulnerabilities found in the new dependencies
All checked dependencies are currently free from known security vulnerabilities in GitHub's Security Advisory database.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories in the new dependencies # Check GitHub Security Advisories for the new dependencies for pkg in "google/go-querystring" "hashicorp/go-cleanhttp" "hashicorp/go-retryablehttp" "hashicorp/go-slug" "hashicorp/go-tfe" "hashicorp/go-version" "hashicorp/jsonapi"; do echo "Checking $pkg..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "'$pkg'") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' doneLength of output: 4407
26-26: Verify the compatibility of utility dependencies.The new utility dependencies look appropriate for the task:
go-querystring: Useful for API requestsgolang.org/x/sync: Essential for synchronization operationsgolang.org/x/time: Helpful for rate limiting API callsHowever, let's verify the compatibility of these versions with the project's Go version (1.22.1).
Also applies to: 54-54, 55-55, 57-57
✅ Verification successful
Dependencies are compatible with Go 1.22.1
The utility dependencies are verified to be compatible:
go-querystring v1.1.0is a stable release from Googlegolang.org/x/syncandgolang.org/x/timeare official Go packages designed for cross-version compatibility🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the dependencies are compatible with Go 1.22.1 # Check go.mod files in these dependencies for their Go version requirements for pkg in "github.com/google/go-querystring" "golang.org/x/sync" "golang.org/x/time"; do echo "Checking $pkg..." gh api repos/${pkg/github.com\//}/contents/go.mod --raw 2>/dev/null | grep -E "^go [0-9]+\.[0-9]+(\.[0-9]+)?$" || echo "go.mod not found for $pkg" doneLength of output: 1089
Script:
#!/bin/bash # Get detailed information about the dependencies echo "Checking go-querystring..." go list -m -json github.com/google/go-querystring@v1.1.0 2>/dev/null || echo "Failed to fetch go-querystring info" echo -e "\nChecking golang.org/x/sync..." go list -m -json golang.org/x/sync 2>/dev/null || echo "Failed to fetch sync info" echo -e "\nChecking golang.org/x/time..." go list -m -json golang.org/x/time 2>/dev/null || echo "Failed to fetch time info"Length of output: 548
| func getWorkspaceVcsRepo(workspace *tfe.Workspace) map[string]string { | ||
| vcsRepo := make(map[string]string) | ||
| if workspace.VCSRepo != nil { | ||
| vcsRepo["terraform-cloud/vcs-repo/identifier"] = workspace.VCSRepo.Identifier | ||
| vcsRepo["terraform-cloud/vcs-repo/branch"] = workspace.VCSRepo.Branch | ||
| vcsRepo["terraform-cloud/vcs-repo/repository-http-url"] = workspace.VCSRepo.RepositoryHTTPURL | ||
| } | ||
| return vcsRepo | ||
| } |
There was a problem hiding this comment.
You should create a new struct call WorkspaceResource or something that "extends the tfe.Workspace and add these functions to that
| if workspace == nil { | ||
| return WorkspaceResource{}, fmt.Errorf("workspace is nil") | ||
| } | ||
| version := workspace.TerraformVersion |
There was a problem hiding this comment.
This is the version of the Kind, which is also static
| variables := getWorkspaceVariables(workspace) | ||
| for key, value := range variables { | ||
| metadata[key] = value | ||
| } | ||
|
|
||
| tags := getWorkspaceTags(workspace) | ||
| for key, value := range tags { | ||
| metadata[key] = value | ||
| } | ||
|
|
||
| vcsRepo := getWorkspaceVcsRepo(workspace) | ||
| for key, value := range vcsRepo { | ||
| metadata[key] = value | ||
| } |
There was a problem hiding this comment.
simplify
moreValues := []map[string]string{
getWorkspaceVariables(workspace),
getWorkspaceTags(workspace),
getWorkspaceVcsRepo(workspace),
}
for k, v := range moreValues {
metdata[k] = v
}
| workspaces := items.Items | ||
| return workspaces, nil |
There was a problem hiding this comment.
| workspaces := items.Items | |
| return workspaces, nil | |
| return items.Items, nil |
|
|
||
| func getWorkspacesInOrg(client *tfe.Client, organization string) ([]*tfe.Workspace, error) { | ||
|
|
||
| // TODO: use cmd context |
There was a problem hiding this comment.
none is going to ever fix this, just delete the comment or do it the right way. not hard to pass in ctx
cmd/ctrlc/root/sync/tfe/tfe.go
Outdated
| tfeClient, err := tfe.NewClient(tfe.DefaultConfig()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create TFE client: %w", err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
| tfeClient, err := tfe.NewClient(tfe.DefaultConfig()) | |
| if err != nil { | |
| return fmt.Errorf("failed to create TFE client: %w", err) | |
| } | |
| return nil | |
| if _, err := tfe.NewClient(tfe.DefaultConfig()); err != nil { | |
| return fmt.Errorf("failed to create TFE client: %w", err) | |
| } | |
| return nil |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
cmd/ctrlc/root/sync/terraform/terraform.go (3)
16-16: RenameworkspaceIdtoworkspaceIDfor Go naming convention consistency.According to Go naming conventions, acronyms and initialisms should be capitalized. Renaming
workspaceIdtoworkspaceIDaligns with these conventions and improves code readability.Apply this diff and update all occurrences:
-var workspaceId string +var workspaceID string
62-62: RenameproviderIdtoproviderIDfor Go naming convention consistency.As per Go conventions,
Idshould be uppercased asID. This change enhances code clarity and adheres to standard practices.Apply this diff and update all occurrences:
-providerId := resp.JSON200.Id +providerID := resp.JSON200.Id
69-76: Define a named type for the resource struct to improve readability and reusability.The inline definition of the resource struct can be replaced with a named type for better clarity and potential reuse elsewhere in the codebase.
Define the struct as a named type:
type Resource struct { Config map[string]interface{} `json:"config"` Identifier string `json:"identifier"` Kind string `json:"kind"` Metadata map[string]string `json:"metadata"` Name string `json:"name"` Version string `json:"version"` }Then update the code to use the new type:
-resources := []struct { - Config map[string]interface{} `json:"config"` - Identifier string `json:"identifier"` - Kind string `json:"kind"` - Metadata map[string]string `json:"metadata"` - Name string `json:"name"` - Version string `json:"version"` -}{} +resources := []Resource{}And within the loop:
-resource := struct { - Config map[string]interface{} `json:"config"` - Identifier string `json:"identifier"` - Kind string `json:"kind"` - Metadata map[string]string `json:"metadata"` - Name string `json:"name"` - Version string `json:"version"` -}{ +resource := Resource{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/ctrlc/root/sync/sync.go(1 hunks)cmd/ctrlc/root/sync/terraform/terraform-org-workspaces.go(1 hunks)cmd/ctrlc/root/sync/terraform/terraform.go(1 hunks)go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ctrlc/root/sync/sync.go
- go.mod
| fmt.Println("Syncing Terraform resources into Ctrlplane") | ||
| apiURL := viper.GetString("url") | ||
| apiKey := viper.GetString("api-key") | ||
| ctx := cmd.Context() | ||
| if _, err := uuid.Parse(workspaceId); err != nil { | ||
| return fmt.Errorf("invalid workspace ID: %w", err) | ||
| } | ||
|
|
||
| ctrlplaneClient, err := api.NewAPIKeyClientWithResponses(apiURL, apiKey) |
There was a problem hiding this comment.
Add validation for the 'organization' flag to prevent empty values.
Currently, there is no validation to ensure that the --organization flag is provided. If it's empty, this can cause errors when constructing the provider name or fetching workspaces.
Apply this diff to add the validation:
ctx := cmd.Context()
+if organization == "" {
+ return fmt.Errorf("organization name must be provided")
+}
if _, err := uuid.Parse(workspaceId); err != nil {
return fmt.Errorf("invalid workspace ID: %w", err)
}Committable suggestion skipped: line range outside the PR's diff.
| apiKey := viper.GetString("api-key") | ||
| ctx := cmd.Context() | ||
| if _, err := uuid.Parse(workspaceId); err != nil { | ||
| return fmt.Errorf("invalid workspace ID: %w", err) | ||
| } | ||
|
|
||
| ctrlplaneClient, err := api.NewAPIKeyClientWithResponses(apiURL, apiKey) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create API client: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for 'apiURL' and 'apiKey' to ensure they are set.
There's no explicit check to ensure that apiURL and apiKey are not empty before creating the API client. If these values are not set, api.NewAPIKeyClientWithResponses might fail unexpectedly.
Consider adding validation:
apiURL := viper.GetString("url")
apiKey := viper.GetString("api-key")
+if apiURL == "" || apiKey == "" {
+ return fmt.Errorf("API URL and API Key must be configured")
+}
ctx := cmd.Context()📝 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.
| apiKey := viper.GetString("api-key") | |
| ctx := cmd.Context() | |
| if _, err := uuid.Parse(workspaceId); err != nil { | |
| return fmt.Errorf("invalid workspace ID: %w", err) | |
| } | |
| ctrlplaneClient, err := api.NewAPIKeyClientWithResponses(apiURL, apiKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to create API client: %w", err) | |
| } | |
| apiKey := viper.GetString("api-key") | |
| if apiURL == "" || apiKey == "" { | |
| return fmt.Errorf("API URL and API Key must be configured") | |
| } | |
| ctx := cmd.Context() | |
| if _, err := uuid.Parse(workspaceId); err != nil { | |
| return fmt.Errorf("invalid workspace ID: %w", err) | |
| } | |
| ctrlplaneClient, err := api.NewAPIKeyClientWithResponses(apiURL, apiKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to create API client: %w", err) | |
| } |
There was a problem hiding this comment.
client will error with invalid creds
There was a problem hiding this comment.
@adityachoudhari26 You're right. The client will handle invalid credentials by returning an error, which is already being caught and handled appropriately. I'll withdraw my suggestion.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/cli#4
File: cmd/ctrlc/root/sync/terraform/terraform.go:35-45
Timestamp: 2025-01-15T17:46:32.055Z
Learning: In the Ctrlplane CLI, the API client (`api.NewAPIKeyClientWithResponses`) handles invalid credentials internally by returning an error, making explicit validation of apiURL and apiKey unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| "ctrlplane/external-id": workspace.ID, | ||
| "terraform-cloud/organization": workspace.Organization.Name, | ||
| "terraform-cloud/workspace-name": workspace.Name, |
There was a problem hiding this comment.
Check for nil workspace.Organization before accessing its Name field.
Accessing workspace.Organization.Name without verifying that workspace.Organization is not nil can lead to a nil pointer dereference and runtime panic.
Add a nil check before accessing workspace.Organization.Name:
metadata := map[string]string{
"ctrlplane/external-id": workspace.ID,
- "terraform-cloud/organization": workspace.Organization.Name,
"terraform-cloud/workspace-name": workspace.Name,
"terraform-cloud/workspace-auto-apply": strconv.FormatBool(workspace.AutoApply),
"terraform/version": workspace.TerraformVersion,
}
+if workspace.Organization != nil {
+ metadata["terraform-cloud/organization"] = workspace.Organization.Name
+}Committable suggestion skipped: line range outside the PR's diff.
| func convertWorkspaceToResource(workspace *tfe.Workspace, baseURL url.URL) (WorkspaceResource, error) { | ||
| if workspace == nil { | ||
| return WorkspaceResource{}, fmt.Errorf("workspace is nil") | ||
| } | ||
| version := Version | ||
| kind := Kind | ||
| name := workspace.Name | ||
| identifier := workspace.ID | ||
| config := map[string]interface{}{ | ||
| "workspaceId": workspace.ID, | ||
| } | ||
| metadata := map[string]string{ | ||
| "ctrlplane/external-id": workspace.ID, | ||
| "terraform-cloud/organization": workspace.Organization.Name, | ||
| "terraform-cloud/workspace-name": workspace.Name, | ||
| "terraform-cloud/workspace-auto-apply": strconv.FormatBool(workspace.AutoApply), | ||
| "terraform/version": workspace.TerraformVersion, | ||
| } | ||
|
|
||
| linksMetadata := getLinksMetadata(workspace, baseURL) | ||
| if linksMetadata != nil { | ||
| metadata["ctrlplane/links"] = *linksMetadata | ||
| } | ||
|
|
||
| moreValues := []map[string]string{ | ||
| getWorkspaceVariables(workspace), | ||
| getWorkspaceTags(workspace), | ||
| getWorkspaceVcsRepo(workspace), | ||
| } | ||
|
|
||
| for _, moreValue := range moreValues { | ||
| for key, value := range moreValue { | ||
| metadata[key] = value | ||
| } | ||
| } | ||
|
|
||
| return WorkspaceResource{ | ||
| Version: version, | ||
| Kind: kind, | ||
| Name: name, | ||
| Identifier: identifier, | ||
| Config: config, | ||
| Metadata: metadata, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential nil values and improve error messages in convertWorkspaceToResource.
Ensure that all accessed fields are checked for nil to prevent runtime panics. Additionally, provide more descriptive error messages when failures occur.
Consider enhancing the function as follows:
- Check for nil
workspace.Organizationbefore accessing its fields. - Provide more context in error messages.
func convertWorkspaceToResource(workspace *tfe.Workspace, baseURL url.URL) (WorkspaceResource, error) {
if workspace == nil {
return WorkspaceResource{}, fmt.Errorf("workspace is nil")
}
+ if workspace.Organization == nil {
+ return WorkspaceResource{}, fmt.Errorf("workspace organization is nil for workspace ID: %s", workspace.ID)
+ }
// ... rest of the function
}📝 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.
| func convertWorkspaceToResource(workspace *tfe.Workspace, baseURL url.URL) (WorkspaceResource, error) { | |
| if workspace == nil { | |
| return WorkspaceResource{}, fmt.Errorf("workspace is nil") | |
| } | |
| version := Version | |
| kind := Kind | |
| name := workspace.Name | |
| identifier := workspace.ID | |
| config := map[string]interface{}{ | |
| "workspaceId": workspace.ID, | |
| } | |
| metadata := map[string]string{ | |
| "ctrlplane/external-id": workspace.ID, | |
| "terraform-cloud/organization": workspace.Organization.Name, | |
| "terraform-cloud/workspace-name": workspace.Name, | |
| "terraform-cloud/workspace-auto-apply": strconv.FormatBool(workspace.AutoApply), | |
| "terraform/version": workspace.TerraformVersion, | |
| } | |
| linksMetadata := getLinksMetadata(workspace, baseURL) | |
| if linksMetadata != nil { | |
| metadata["ctrlplane/links"] = *linksMetadata | |
| } | |
| moreValues := []map[string]string{ | |
| getWorkspaceVariables(workspace), | |
| getWorkspaceTags(workspace), | |
| getWorkspaceVcsRepo(workspace), | |
| } | |
| for _, moreValue := range moreValues { | |
| for key, value := range moreValue { | |
| metadata[key] = value | |
| } | |
| } | |
| return WorkspaceResource{ | |
| Version: version, | |
| Kind: kind, | |
| Name: name, | |
| Identifier: identifier, | |
| Config: config, | |
| Metadata: metadata, | |
| }, nil | |
| } | |
| func convertWorkspaceToResource(workspace *tfe.Workspace, baseURL url.URL) (WorkspaceResource, error) { | |
| if workspace == nil { | |
| return WorkspaceResource{}, fmt.Errorf("workspace is nil") | |
| } | |
| if workspace.Organization == nil { | |
| return WorkspaceResource{}, fmt.Errorf("workspace organization is nil for workspace ID: %s", workspace.ID) | |
| } | |
| version := Version | |
| kind := Kind | |
| name := workspace.Name | |
| identifier := workspace.ID | |
| config := map[string]interface{}{ | |
| "workspaceId": workspace.ID, | |
| } | |
| metadata := map[string]string{ | |
| "ctrlplane/external-id": workspace.ID, | |
| "terraform-cloud/organization": workspace.Organization.Name, | |
| "terraform-cloud/workspace-name": workspace.Name, | |
| "terraform-cloud/workspace-auto-apply": strconv.FormatBool(workspace.AutoApply), | |
| "terraform/version": workspace.TerraformVersion, | |
| } | |
| linksMetadata := getLinksMetadata(workspace, baseURL) | |
| if linksMetadata != nil { | |
| metadata["ctrlplane/links"] = *linksMetadata | |
| } | |
| moreValues := []map[string]string{ | |
| getWorkspaceVariables(workspace), | |
| getWorkspaceTags(workspace), | |
| getWorkspaceVcsRepo(workspace), | |
| } | |
| for _, moreValue := range moreValues { | |
| for key, value := range moreValue { | |
| metadata[key] = value | |
| } | |
| } | |
| return WorkspaceResource{ | |
| Version: version, | |
| Kind: kind, | |
| Name: name, | |
| Identifier: identifier, | |
| Config: config, | |
| Metadata: metadata, | |
| }, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/ctrlc/root/sync/terraform/terraform-org-workspaces.go (2)
39-41: Use consistent error handling and consider returning errorsIn the
getLinksMetadatafunction, when JSON marshaling fails, the error is logged but not returned. This might hide issues that could be important for upstream error handling or for informing the user about what went wrong.Consider modifying the function to return the error for better error propagation:
func getLinksMetadata(workspace *tfe.Workspace, baseURL url.URL) (*string, error) { if workspace.Organization == nil { return nil, nil } links := map[string]string{ "Terraform Workspace": fmt.Sprintf("%s/app/%s/workspaces/%s", baseURL.String(), workspace.Organization.Name, workspace.Name), } linksJSON, err := json.Marshal(links) if err != nil { - log.Error("Failed to marshal links", "error", err) - return nil + return nil, fmt.Errorf("failed to marshal links: %w", err) } linksString := string(linksJSON) - return &linksString + return &linksString, nil }Update the calling function
convertWorkspaceToResourceto handle this error:linksMetadata, err := getLinksMetadata(workspace, baseURL) -if linksMetadata != nil { +if err != nil { + return WorkspaceResource{}, fmt.Errorf("failed to get links metadata: %w", err) +} +if linksMetadata != nil { metadata["ctrlplane/links"] = *linksMetadata }
50-53: Ensure variable keys are properly namespaced and handle potential key collisionsIn the
getWorkspaceVariablesfunction, all variable keys are prefixed withterraform-cloud/variables/, but there could be a risk of key collisions if different variables share the same name across different workspaces.Consider including the workspace name or ID in the key to uniquely identify variables:
func getWorkspaceVariables(workspace *tfe.Workspace) map[string]string { variables := make(map[string]string) for _, variable := range workspace.Variables { if variable != nil && variable.Category == tfe.CategoryTerraform && !variable.Sensitive { - key := fmt.Sprintf("terraform-cloud/variables/%s", variable.Key) + key := fmt.Sprintf("terraform-cloud/variables/%s/%s", workspace.Name, variable.Key) variables[key] = variable.Value } } return variables }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/ctrlc/root/sync/terraform/terraform-org-workspaces.go(1 hunks)cmd/ctrlc/root/sync/terraform/terraform.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ctrlc/root/sync/terraform/terraform.go
🔇 Additional comments (2)
cmd/ctrlc/root/sync/terraform/terraform-org-workspaces.go (2)
95-97: Great job checking forworkspace.Organizationbefore accessing itsNamefieldYou have appropriately added a nil check for
workspace.Organizationbefore accessingworkspace.Organization.Name, which prevents potential nil pointer dereferences.
174-180: [Duplicate Comment] Handle potential nil values and improve error handling inconvertWorkspaceToResourceThis concern was previously mentioned in past reviews and remains important to ensure robustness.
| for _, tag := range workspace.Tags { | ||
| key := fmt.Sprintf("terraform-cloud/tag/%s", tag.Name) | ||
| tags[key] = "true" | ||
| } |
There was a problem hiding this comment.
Add nil check for tag to prevent potential nil pointer dereference
In the getWorkspaceTags function, if any of the tag elements in workspace.Tags are nil, accessing tag.Name will cause a nil pointer dereference and lead to a runtime panic.
Apply this diff to add a nil check for tag:
func getWorkspaceTags(workspace *tfe.Workspace) map[string]string {
tags := make(map[string]string)
for _, tag := range workspace.Tags {
+ if tag == nil {
+ continue
+ }
key := fmt.Sprintf("terraform-cloud/tag/%s", tag.Name)
tags[key] = "true"
}
return tags
}📝 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.
| for _, tag := range workspace.Tags { | |
| key := fmt.Sprintf("terraform-cloud/tag/%s", tag.Name) | |
| tags[key] = "true" | |
| } | |
| for _, tag := range workspace.Tags { | |
| if tag == nil { | |
| continue | |
| } | |
| key := fmt.Sprintf("terraform-cloud/tag/%s", tag.Name) | |
| tags[key] = "true" | |
| } |
| for _, workspace := range workspaces { | ||
| workspaceResource, err := convertWorkspaceToResource(workspace, client.BaseURL()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| workspaceResources = append(workspaceResources, workspaceResource) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors during workspace conversion to prevent premature termination
In the getWorkspacesInOrg function, if convertWorkspaceToResource returns an error for a single workspace, the entire process stops and returns the error. This means that if one workspace fails to convert, none of the subsequent workspaces are processed, which could hinder the synchronization of valid workspaces.
Consider logging the error and continuing with the next workspace to ensure all valid workspaces are processed:
for _, workspace := range workspaces {
workspaceResource, err := convertWorkspaceToResource(workspace, client.BaseURL())
if err != nil {
- return nil, err
+ log.Error("Failed to convert workspace to resource", "workspace_id", workspace.ID, "error", err)
+ continue
}
workspaceResources = append(workspaceResources, workspaceResource)
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
docker/README.md (2)
15-17: Add version guidance for the Docker image.Consider adding guidance about available versions or how users can find the latest version. This helps users make informed decisions about which version to use.
```sh docker pull ctrlplane/cli:v0.1.0
+For a list of available versions, visit our Docker Hub repository at https://hub.docker.com/r/ctrlplane/cli/tags
--- `21-23`: **Enhance run instructions with common command examples.** The placeholder `[your-command]` might not provide enough guidance. Consider adding examples of common commands to help users get started. ```diff ```sh docker run ctrlplane/cli ctrlc [your-command]
+Common commands:
+sh +# List available commands +docker run ctrlplane/cli ctrlc --help + +# Get version information +docker run ctrlplane/cli ctrlc version +</blockquote></details> <details> <summary>docker/Dockerfile (1)</summary><blockquote> `1-3`: **Consider version pinning strategy and default CLI version.** While using Alpine is good for minimal image size, consider: 1. Pin the Alpine version more specifically (e.g., `3.19.0`) for better reproducibility 2. Provide a default value for `CLI_VERSION` to improve usability ```diff -FROM alpine:3.19 +FROM alpine:3.19.0 ARG CLI_VERSION +ARG CLI_VERSION="v0.1.0" # Provide appropriate default version.github/workflows/release.yaml (4)
34-42: LGTM! Job definition and permissions are well-structured.The permissions are correctly scoped, and the repository owner check is consistent with other jobs.
Remove trailing spaces on lines 36 and 38 to fix yamllint warnings.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
43-46: Consider adding support for arm64 architecture.The matrix currently only builds for
linux/amd64. Consider addinglinux/arm64to support ARM-based systems, which are increasingly common in cloud environments.matrix: platform: [linux/amd64] + # Consider adding: linux/arm64
57-71: Improve shell script robustness.The Docker Hub authentication logic is well-structured, but the shell script could be more robust.
- name: Check if Docker Hub secrets are available run: | - if [ -z "${{ secrets.DOCKERHUB_USERNAME }}" ] || [ -z "${{ secrets.DOCKERHUB_TOKEN }}" ]; then - echo "DOCKERHUB_LOGIN=false" >> $GITHUB_ENV + if [ -z "${DOCKERHUB_USERNAME}" ] || [ -z "${DOCKERHUB_TOKEN}" ]; then + echo "DOCKERHUB_LOGIN=false" >> "${GITHUB_ENV}" else - echo "DOCKERHUB_LOGIN=true" >> $GITHUB_ENV + echo "DOCKERHUB_LOGIN=true" >> "${GITHUB_ENV}" fi env: + DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} + DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }}🧰 Tools
🪛 actionlint (1.7.4)
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
34-35: Consider adding job dependency.The
dockerjob should ideally wait for thelinuxjob to complete since it's part of the same release process.docker: runs-on: ubuntu-latest + needs: linux
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release.yaml(1 hunks)docker/Dockerfile(1 hunks)docker/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docker/README.md
[uncategorized] ~27-~27: Loose punctuation mark.
Context: ...ronment variables - CTRLPLANE_API_KEY: Your Ctrlplane API key. - `CTRLPLANE_UR...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ...our Ctrlplane API key. - CTRLPLANE_URL: The URL of your Ctrlplane instance (e.g...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~31-~31: Consider a shorter alternative to avoid wordiness.
Context: ...p.ctrlplane.dev`). ### Terraform sync In order to sync Terraform resources into Ctrlplane...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...ng environment variables: - TFE_TOKEN: Your Terraform Cloud API token. - `TFE_...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
74-74: the runner of "docker/metadata-action@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/release.yaml (1)
47-56: LGTM! Setup steps follow best practices.The setup includes all necessary components (checkout, QEMU, Buildx) with recent action versions.
| ### Required environment variables | ||
|
|
||
| - `CTRLPLANE_API_KEY`: Your Ctrlplane API key. | ||
| - `CTRLPLANE_URL`: The URL of your Ctrlplane instance (e.g. `https://app.ctrlplane.dev`). | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add examples of passing environment variables to Docker.
Show users how to properly pass the required environment variables to the Docker container.
### Required environment variables
- `CTRLPLANE_API_KEY`: Your Ctrlplane API key.
- `CTRLPLANE_URL`: The URL of your Ctrlplane instance (e.g. `https://app.ctrlplane.dev`).
+
+Example using environment variables:
+```sh
+docker run \
+ -e CTRLPLANE_API_KEY=your-api-key \
+ -e CTRLPLANE_URL=https://app.ctrlplane.dev \
+ ctrlplane/cli ctrlc [command]
+```
+
+Example using a .env file:
+```sh
+docker run --env-file .env ctrlplane/cli ctrlc [command]
+```🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: Loose punctuation mark.
Context: ...ronment variables - CTRLPLANE_API_KEY: Your Ctrlplane API key. - `CTRLPLANE_UR...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ...our Ctrlplane API key. - CTRLPLANE_URL: The URL of your Ctrlplane instance (e.g...
(UNLIKELY_OPENING_PUNCTUATION)
| ### Terraform sync | ||
|
|
||
| In order to sync Terraform resources into Ctrlplane, you need to set the following environment variables: | ||
|
|
||
| - `TFE_TOKEN`: Your Terraform Cloud API token. | ||
| - `TFE_ADDRESS` (optional): The URL of your Terraform Cloud instance (e.g. `https://app.terraform.io`). If not set, the default address (`https://app.terraform.io`) is used. | ||
|
|
||
| ```sh | ||
| docker run ctrlplane/cli ctrlc sync terraform --organization my-org --workspace-id 2a7c5560-75c9-4dbe-be74-04ee33bf8188 | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Terraform sync documentation with parameter explanations.
The current example lacks context about the required parameters and how to obtain them.
### Terraform sync
In order to sync Terraform resources into Ctrlplane, you need to set the following environment variables:
- `TFE_TOKEN`: Your Terraform Cloud API token.
- `TFE_ADDRESS` (optional): The URL of your Terraform Cloud instance (e.g. `https://app.terraform.io`). If not set, the default address (`https://app.terraform.io`) is used.
+Required parameters:
+- `--organization`: Your Terraform Cloud organization name
+- `--workspace-id`: The ID of the workspace to sync (found in workspace settings or via Terraform Cloud API)
+
```sh
-docker run ctrlplane/cli ctrlc sync terraform --organization my-org --workspace-id 2a7c5560-75c9-4dbe-be74-04ee33bf8188
+# Example with all environment variables
+docker run \
+ -e CTRLPLANE_API_KEY=your-api-key \
+ -e CTRLPLANE_URL=https://app.ctrlplane.dev \
+ -e TFE_TOKEN=your-terraform-token \
+ ctrlplane/cli ctrlc sync terraform \
+ --organization my-org \
+ --workspace-id 2a7c5560-75c9-4dbe-be74-04ee33bf8188+For more information about finding your organization name and workspace ID, visit:
+- Organization: https://app.terraform.io/app/organizations
+- Workspace ID: Navigate to workspace settings in Terraform Cloud UI
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 LanguageTool</summary>
[style] ~31-~31: Consider a shorter alternative to avoid wordiness.
Context: ...p.ctrlplane.dev`). ### Terraform sync In order to sync Terraform resources into Ctrlplane...
(IN_ORDER_TO_PREMIUM)
---
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...ng environment variables: - `TFE_TOKEN`: Your Terraform Cloud API token. - `TFE_...
(UNLIKELY_OPENING_PUNCTUATION)
</details>
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
docker/Dockerfile
Outdated
| RUN apk add --no-cache curl tar && \ | ||
| curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \ | ||
| tar xzf ctrlc.tar.gz && \ | ||
| mv ctrlc /usr/local/bin/ && \ | ||
| chmod +x /usr/local/bin/ctrlc && \ | ||
| rm -rf ctrlc.tar.gz |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add checksum verification and improve error handling.
The current installation process lacks security verification and proper error handling:
- Add checksum verification for the downloaded artifact
- Add version validation
- Consider splitting the RUN command for better layer caching
-RUN apk add --no-cache curl tar && \
- curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \
- tar xzf ctrlc.tar.gz && \
- mv ctrlc /usr/local/bin/ && \
- chmod +x /usr/local/bin/ctrlc && \
- rm -rf ctrlc.tar.gz
+# Install dependencies
+RUN apk add --no-cache curl tar
+
+# Validate CLI_VERSION
+RUN [ -n "$CLI_VERSION" ] || (echo "CLI_VERSION must be set" && false)
+
+# Download and verify CLI
+RUN curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \
+ curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/checksums.txt" -o checksums.txt && \
+ sha256sum --check --ignore-missing checksums.txt && \
+ tar xzf ctrlc.tar.gz && \
+ mv ctrlc /usr/local/bin/ && \
+ chmod +x /usr/local/bin/ctrlc && \
+ rm -rf ctrlc.tar.gz checksums.txt📝 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.
| RUN apk add --no-cache curl tar && \ | |
| curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \ | |
| tar xzf ctrlc.tar.gz && \ | |
| mv ctrlc /usr/local/bin/ && \ | |
| chmod +x /usr/local/bin/ctrlc && \ | |
| rm -rf ctrlc.tar.gz | |
| # Install dependencies | |
| RUN apk add --no-cache curl tar | |
| # Validate CLI_VERSION | |
| RUN [ -n "$CLI_VERSION" ] || (echo "CLI_VERSION must be set" && false) | |
| # Download and verify CLI | |
| RUN curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \ | |
| curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/checksums.txt" -o checksums.txt && \ | |
| sha256sum --check --ignore-missing checksums.txt && \ | |
| tar xzf ctrlc.tar.gz && \ | |
| mv ctrlc /usr/local/bin/ && \ | |
| chmod +x /usr/local/bin/ctrlc && \ | |
| rm -rf ctrlc.tar.gz checksums.txt |
docker/Dockerfile
Outdated
| WORKDIR /app | ||
|
|
||
| CMD ["ctrlc", "--help"] No newline at end of file |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused WORKDIR and improve default command
The /app working directory is not utilized anywhere in the container:
- No files are copied there
- No operations use this path
- Can be safely removed
Consider replacing the default help command with a more meaningful message about how to use the container.
🔗 Analysis chain
Clarify the purpose of the working directory.
The /app working directory is set but not utilized. Consider:
- Document why this directory is needed
- Consider if a default help command is the most useful default
Let's check if this working directory is used elsewhere in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to /app directory
rg "/app" --type-not dockerfile
Length of output: 66
Script:
#!/bin/bash
# Search for references to /app directory, excluding the Dockerfile
rg "/app" --glob '!docker/Dockerfile'
# Check Dockerfile for COPY/ADD commands
cat docker/Dockerfile
Length of output: 1040
| - name: Build and Push | ||
| uses: docker/build-push-action@v6 | ||
| if: github.ref == 'refs/heads/main' && env.DOCKERHUB_LOGIN == 'true' | ||
| with: |
There was a problem hiding this comment.
Fix incorrect branch condition for release workflow.
This is a release workflow triggered by tags (v*), but the build step checks for the main branch. This condition will prevent the Docker image from being built during releases.
- name: Build and Push
uses: docker/build-push-action@v6
- if: github.ref == 'refs/heads/main' && env.DOCKERHUB_LOGIN == 'true'
+ if: startsWith(github.ref, 'refs/tags/v') && env.DOCKERHUB_LOGIN == 'true'📝 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.
| - name: Build and Push | |
| uses: docker/build-push-action@v6 | |
| if: github.ref == 'refs/heads/main' && env.DOCKERHUB_LOGIN == 'true' | |
| with: | |
| - name: Build and Push | |
| uses: docker/build-push-action@v6 | |
| if: startsWith(github.ref, 'refs/tags/v') && env.DOCKERHUB_LOGIN == 'true' | |
| with: |
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GH_TOKEN }} | ||
|
|
||
| docker: |
There was a problem hiding this comment.
Do the push on main, not the build, we can do that on every branch
docker/Dockerfile
Outdated
| RUN apk add --no-cache curl tar && \ | ||
| curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \ | ||
| tar xzf ctrlc.tar.gz && \ | ||
| mv ctrlc /usr/local/bin/ && \ | ||
| chmod +x /usr/local/bin/ctrlc && \ | ||
| rm -rf ctrlc.tar.gz |
There was a problem hiding this comment.
Consider running as a stage to drop curl and tar from the final image
ex:
FROM alpine as Builder
RUN your thing
FROM alpine as Final
COPY --from=Builder /yourthing /usr/bin/yourthing
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docker/Dockerfile (1)
3-9:⚠️ Potential issueAdd checksum verification and improve error handling in the builder stage.
The current installation process lacks security verification and proper error handling:
Apply this diff to add checksum verification and improve error handling:
ARG CLI_VERSION + +# Validate CLI_VERSION +RUN [ -n "$CLI_VERSION" ] || (echo "CLI_VERSION must be set" && false) RUN apk add --no-cache curl tar && \ - curl -L --fail "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \ - tar xzf ctrlc.tar.gz && \ - mv ctrlc /usr/local/bin/ && \ - chmod +x /usr/local/bin/ctrlc + curl -L --fail \ + "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/ctrlc_Linux_x86_64.tar.gz" -o ctrlc.tar.gz && \ + curl -L --fail \ + "https://github.com/ctrlplanedev/cli/releases/download/${CLI_VERSION}/checksums.txt" -o checksums.txt && \ + sha256sum --check --ignore-missing checksums.txt && \ + tar xzf ctrlc.tar.gz && \ + mv ctrlc /usr/local/bin/ && \ + chmod +x /usr/local/bin/ctrlc && \ + rm -rf ctrlc.tar.gz checksums.txt
🧹 Nitpick comments (7)
cmd/ctrlc/root/sync/terraform/terraform-org-workspaces.go (4)
17-20: Add documentation for constants.Consider adding documentation comments to explain the purpose and usage of these constants, especially since they are used in the resource conversion process.
+// Kind represents the resource type for Terraform workspaces const ( Kind = "Workspace" + // Version indicates the API version for Terraform workspace resources Version = "terraform/v1" )
22-29: Add struct documentation and field validation.The struct would benefit from:
- Documentation explaining its purpose and usage
- Field documentation
- Optional: Validation tags for required fields
+// WorkspaceResource represents a Terraform workspace with its configuration and metadata type WorkspaceResource struct { + // Config contains workspace-specific configuration parameters Config map[string]interface{} + // Identifier is the unique identifier for the workspace Identifier string + // Kind indicates the type of resource Kind string + // Metadata contains additional workspace information Metadata map[string]string + // Name is the workspace name Name string + // Version indicates the API version Version string }
79-126: Enhance error context in workspace conversion.While the function is well-implemented, consider providing more context in the error message when workspace is nil.
if workspace == nil { - return WorkspaceResource{}, fmt.Errorf("workspace is nil") + return WorkspaceResource{}, fmt.Errorf("cannot convert nil workspace to resource") }
128-185: Consider making retry parameters configurable.The retry logic is well-implemented, but consider making the retry parameters (attempts, delay, max delay) configurable through function parameters or environment variables for better flexibility in different environments.
+type RetryConfig struct { + Attempts uint + Delay time.Duration + MaxDelay time.Duration +} + +func defaultRetryConfig() RetryConfig { + return RetryConfig{ + Attempts: 5, + Delay: time.Second, + MaxDelay: 5 * time.Second, + } +} + -func listWorkspacesWithRetry(ctx context.Context, client *tfe.Client, organization string, pageNum, pageSize int) (*tfe.WorkspaceList, error) { +func listWorkspacesWithRetry(ctx context.Context, client *tfe.Client, organization string, pageNum, pageSize int, retryConfig *RetryConfig) (*tfe.WorkspaceList, error) { + if retryConfig == nil { + cfg := defaultRetryConfig() + retryConfig = &cfg + } var workspaces *tfe.WorkspaceList err := retry.Do( func() error { var err error workspaces, err = client.Workspaces.List(ctx, organization, &tfe.WorkspaceListOptions{ ListOptions: tfe.ListOptions{ PageNumber: pageNum, PageSize: pageSize, }, }) return err }, - retry.Attempts(5), - retry.Delay(time.Second), - retry.MaxDelay(5*time.Second), + retry.Attempts(retryConfig.Attempts), + retry.Delay(retryConfig.Delay), + retry.MaxDelay(retryConfig.MaxDelay), ) return workspaces, err }docker/Dockerfile (1)
11-15: Consider adding a more meaningful default command.The current default command shows the help message. Consider providing a more descriptive message about container usage.
Apply this diff to improve the default command:
-CMD ["ctrlc", "--help"] +CMD ["sh", "-c", "echo 'Ctrlplane CLI container. Set CTRLPLANE_API_KEY and CTRLPLANE_URL environment variables before running commands.' && ctrlc --help"].github/workflows/release.yaml (2)
83-103: Consolidate build steps and improve conditions.The build steps can be consolidated to reduce duplication and improve maintainability.
Apply this diff to consolidate the build steps:
- - name: Build Only - uses: docker/build-push-action@v6 - if: env.DOCKERHUB_LOGIN != 'true' - with: - push: false - file: docker/Dockerfile - platforms: ${{ matrix.platform }} - tags: ${{ steps.meta.outputs.tags }} - build-args: | - CLI_VERSION=${{ steps.meta.outputs.version }} - - - name: Build and Push + - name: Build and Push Docker image uses: docker/build-push-action@v6 - if: env.DOCKERHUB_LOGIN == 'true' with: - push: true + push: ${{ env.DOCKERHUB_LOGIN == 'true' }} file: docker/Dockerfile platforms: ${{ matrix.platform }} tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} build-args: | CLI_VERSION=${{ steps.meta.outputs.version }}
38-38: Remove trailing spaces.There are trailing spaces in the YAML file.
Apply this diff to remove trailing spaces:
runs-on: ubuntu-latest - + if: github.repository_owner == 'ctrlplanedev' - +Also applies to: 40-40
🧰 Tools
🪛 yamllint (1.35.1)
[error] 38-38: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yaml(2 hunks)cmd/ctrlc/root/sync/terraform/terraform-org-workspaces.go(1 hunks)docker/Dockerfile(1 hunks)docker/README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/README.md
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
60-60: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: the runner of "docker/metadata-action@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 38-38: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
cmd/ctrlc/root/sync/terraform/terraform-org-workspaces.go (2)
1-15: LGTM! Well-organized package structure and imports.The package name and imports are appropriate for the functionality. The code uses relevant external packages for logging, retries, and Terraform Enterprise API integration.
31-77: LGTM! Well-implemented helper functions with proper nil checks.The helper functions are well-structured and include appropriate nil checks to prevent runtime panics. They handle different aspects of workspace metadata effectively:
- Links metadata with organization validation
- Non-sensitive Terraform variables
- VCS repository information
- Workspace tags
| - name: Extract metadata (tags, labels) for Docker | ||
| id: meta | ||
| uses: docker/metadata-action@v4 | ||
| with: | ||
| images: ctrlplane/cli | ||
| tags: | | ||
| type=raw,value=latest | ||
| type=ref,event=tag | ||
There was a problem hiding this comment.
Update metadata-action version and improve tag strategy.
The current version of metadata-action is outdated.
Apply this diff to update the action and improve tag strategy:
- name: Extract metadata (tags, labels) for Docker
id: meta
- uses: docker/metadata-action@v4
+ uses: docker/metadata-action@v5
with:
images: ctrlplane/cli
tags: |
type=raw,value=latest
+ type=semver,pattern={{version}}
+ type=semver,pattern={{major}}.{{minor}}
type=ref,event=tag📝 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.
| - name: Extract metadata (tags, labels) for Docker | |
| id: meta | |
| uses: docker/metadata-action@v4 | |
| with: | |
| images: ctrlplane/cli | |
| tags: | | |
| type=raw,value=latest | |
| type=ref,event=tag | |
| - name: Extract metadata (tags, labels) for Docker | |
| id: meta | |
| uses: docker/metadata-action@v5 | |
| with: | |
| images: ctrlplane/cli | |
| tags: | | |
| type=raw,value=latest | |
| type=semver,pattern={{version}} | |
| type=semver,pattern={{major}}.{{minor}} | |
| type=ref,event=tag |
🧰 Tools
🪛 actionlint (1.7.4)
76-76: the runner of "docker/metadata-action@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - name: Check if Docker Hub secrets are available | ||
| run: | | ||
| if [ -z "${{ secrets.DOCKERHUB_USERNAME }}" ] || [ -z "${{ secrets.DOCKERHUB_TOKEN }}" ]; then | ||
| echo "DOCKERHUB_LOGIN=false" >> $GITHUB_ENV | ||
| else | ||
| echo "DOCKERHUB_LOGIN=true" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
Fix shell script quoting in Docker Hub secrets check.
The shell script has potential word splitting issues.
Apply this diff to fix the shell script:
- name: Check if Docker Hub secrets are available
run: |
- if [ -z "${{ secrets.DOCKERHUB_USERNAME }}" ] || [ -z "${{ secrets.DOCKERHUB_TOKEN }}" ]; then
+ if [ -z "${DOCKERHUB_USERNAME}" ] || [ -z "${DOCKERHUB_TOKEN}" ]; then
echo "DOCKERHUB_LOGIN=false" >> $GITHUB_ENV
else
echo "DOCKERHUB_LOGIN=true" >> $GITHUB_ENV
fi
+ env:
+ DOCKERHUB_USERNAME: "${{ secrets.DOCKERHUB_USERNAME }}"
+ DOCKERHUB_TOKEN: "${{ secrets.DOCKERHUB_TOKEN }}"📝 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.
| - name: Check if Docker Hub secrets are available | |
| run: | | |
| if [ -z "${{ secrets.DOCKERHUB_USERNAME }}" ] || [ -z "${{ secrets.DOCKERHUB_TOKEN }}" ]; then | |
| echo "DOCKERHUB_LOGIN=false" >> $GITHUB_ENV | |
| else | |
| echo "DOCKERHUB_LOGIN=true" >> $GITHUB_ENV | |
| fi | |
| - name: Check if Docker Hub secrets are available | |
| run: | | |
| if [ -z "${DOCKERHUB_USERNAME}" ] || [ -z "${DOCKERHUB_TOKEN}" ]; then | |
| echo "DOCKERHUB_LOGIN=false" >> $GITHUB_ENV | |
| else | |
| echo "DOCKERHUB_LOGIN=true" >> $GITHUB_ENV | |
| fi | |
| env: | |
| DOCKERHUB_USERNAME: "${{ secrets.DOCKERHUB_USERNAME }}" | |
| DOCKERHUB_TOKEN: "${{ secrets.DOCKERHUB_TOKEN }}" |
🧰 Tools
🪛 actionlint (1.7.4)
60-60: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
60-60: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/release.yaml (2)
57-63:⚠️ Potential issueFix shell script quoting in Docker Hub secrets check.
The shell script has potential word splitting issues.
- name: Check if Docker Hub secrets are available run: | - if [ -z "${{ secrets.DOCKERHUB_USERNAME }}" ] || [ -z "${{ secrets.DOCKERHUB_TOKEN }}" ]; then + if [ -z "${DOCKERHUB_USERNAME}" ] || [ -z "${DOCKERHUB_TOKEN}" ]; then echo "DOCKERHUB_LOGIN=false" >> $GITHUB_ENV else echo "DOCKERHUB_LOGIN=true" >> $GITHUB_ENV fi + env: + DOCKERHUB_USERNAME: "${{ secrets.DOCKERHUB_USERNAME }}" + DOCKERHUB_TOKEN: "${{ secrets.DOCKERHUB_TOKEN }}"🧰 Tools
🪛 actionlint (1.7.4)
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
72-80:⚠️ Potential issueUpdate metadata-action version and improve tag strategy.
The current version of metadata-action is outdated.
- name: Extract metadata (tags, labels) for Docker id: meta - uses: docker/metadata-action@v4 + uses: docker/metadata-action@v5 with: images: ctrlplane/cli tags: | type=raw,value=latest + type=semver,pattern={{version}} + type=semver,pattern={{major}}.{{minor}} type=ref,event=tag🧰 Tools
🪛 actionlint (1.7.4)
74-74: the runner of "docker/metadata-action@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (2)
.github/workflows/release.yaml (2)
36-36: Remove trailing spaces.Remove trailing spaces from lines 36 and 38 for cleaner code.
- +Also applies to: 38-38
🧰 Tools
🪛 yamllint (1.35.1)
[error] 36-36: trailing spaces
(trailing-spaces)
43-46: Consider supporting additional platforms.The platform matrix currently only supports
linux/amd64. Consider adding support for other architectures likelinux/arm64for broader compatibility.strategy: matrix: - platform: [linux/amd64] + platform: [linux/amd64, linux/arm64]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:4:34: Double quote to prevent globbing and word splitting
(shellcheck)
74-74: the runner of "docker/metadata-action@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/release.yaml (2)
48-56: LGTM! Docker setup steps are well configured.The setup steps use appropriate action versions and follow best practices.
81-101: LGTM! Build steps are well structured.The build steps are correctly configured to handle both scenarios:
- Build only when Docker Hub credentials are not available
- Build and push when credentials are available
Summary by CodeRabbit
New Features
Dependencies
Improvements