Add component tests #17
Conversation
WalkthroughThis pull request introduces several modifications to the Changes
Poem
✨ Finishing Touches
🪧 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 (
|
|
/terratest |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Require terratestWonderful, this rule succeeded.This rule require terratest status
|
|
/terratest |
1 similar comment
|
/terratest |
…s/aws-eks-actions-runner-controller into add-tests * 'add-tests' of github.com:cloudposse-terraform-components/aws-eks-actions-runner-controller: chore: update README.md (#18)
|
/terratest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/component_test.go (2)
51-51: Fix the typo in the namespace name.
Currently spelled "acttions-runners-%s" instead of "actions-runners-%s". A minor correction ensures consistent naming.- namespace := fmt.Sprintf("acttions-runners-%s", randomID) + namespace := fmt.Sprintf("actions-runners-%s", randomID)
148-151: Use test assertions instead of panic for improved clarity.
Panic will stop test execution abruptly. Consider usingrequire.NoErrororassert.FailNowfor a standard test failure pattern.- if err != nil { - panic(fmt.Errorf("failed to create dynamic client: %v", err)) - } + assert.NoError(s.T(), err, fmt.Sprintf("failed to create dynamic client: %v", err))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/component_test.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
test/component_test.go
165-165: Error return value of informer.AddEventHandler is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (5)
test/component_test.go (5)
9-9: Import of the 'time' package looks good.
This addition is necessary for timeout handling and appears correctly utilized in the subsequent logic.
20-26: Reintroduced Kubernetes imports are consistent with the new informer logic.
They match the usage below for the dynamic client, GVR, and shared informer factory.
144-146: Configuration check logic is clear and well-handled.
AssertingNoErrorandNotNilmakes sense for verifying the K8S config.
153-158: Defining the Runners CRD GVR looks good.
This matches the custom resource group/version foractions.summerwind.dev.
160-161: Dynamic informer factory creation is correct.
Usingcorev1.NamespaceAlland a re-sync period of one minute is appropriate for broad-based resource tracking.
|
/terratest |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/component_test.go (1)
188-190:⚠️ Potential issueAddress potential concurrency issue when closing stopChannel.
Multiple update events could attempt to close the channel more than once, leading to a panic. You should implement a safety check before closing the channel.
- if phase == "Running" && ready == true { - close(stopChannel) // Stop the informer if the external secret is ready + if phase == "Running" && ready == true { + select { + case <-stopChannel: + // Channel is already closed, prevent panic + return + default: + close(stopChannel) // Stop the informer if the runner is ready + } }
🧹 Nitpick comments (5)
test/component_test.go (5)
51-51: Fix typo in namespace name.There appears to be a typo in the namespace name: "acttions-runners-%s" should be "actions-runners-%s".
- namespace := fmt.Sprintf("acttions-runners-%s", randomID) + namespace := fmt.Sprintf("actions-runners-%s", randomID)
149-151: Use consistent error handling pattern.The error handling approach here (using panic) is inconsistent with the rest of the code that uses
assert.NoError. Consider using the same error handling pattern for consistency.- if err != nil { - panic(fmt.Errorf("failed to create dynamic client: %v", err)) - } + assert.NoError(s.T(), err, "failed to create dynamic client") + assert.NotNil(s.T(), dynamicClient)
196-204: Improve resource cleanup in timeout case.In the timeout case, you're using a defer to close the stopChannel, but this could lead to a panic if the channel was already closed by the event handler. Also, the comment mentions "external secret" which seems out of context.
select { case <-stopChannel: - msg := "runner is ready" + msg := "Runner is ready" fmt.Println(msg) case <-time.After(5 * time.Minute): - defer close(stopChannel) + // Safely close the channel if not already closed + select { + case <-stopChannel: + // Channel is already closed + default: + close(stopChannel) + } - msg := "runner is not ready" + msg := "Runner is not ready after 5 minutes" assert.Fail(s.T(), msg) }
160-192: Add error logging for better debugging.The current error handling for status retrieval only prints to stdout without context. Consider adding more details to the error messages for easier debugging.
if err != nil || !found { - fmt.Println("Error retrieving conditions from status phase") + fmt.Printf("Error retrieving phase from runner %s/%s: %v, found: %t\n", + runner.GetNamespace(), runner.GetName(), err, found) return } // Similar improvement for the ready status error message🧰 Tools
🪛 golangci-lint (1.64.8)
165-165: Error return value of
informer.AddEventHandleris not checked(errcheck)
169-172: Improve readability of runner name check.The current check for the runner name prefixes the message with "runner name is not 'infra-runner'", which can be confusing when the name actually does start with 'infra-runner' but doesn't match for other reasons. Consider making this message more precise.
if !strings.HasPrefix(runner.GetName(), "infra-runner") || runner.GetNamespace() != namespace { - fmt.Printf("runner name is not 'infra-runner', it is '%s'\n", runner.GetName()) + fmt.Printf("Skipping runner %s/%s (waiting for runner with prefix 'infra-runner' in namespace %s)\n", + runner.GetNamespace(), runner.GetName(), namespace) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/component_test.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
test/component_test.go
165-165: Error return value of informer.AddEventHandler is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (3)
test/component_test.go (3)
165-165: Handle the return value from AddEventHandler.The static analysis tool indicates that the return value from
informer.AddEventHandleris not checked. You should explicitly assign or discard it to address the linter warning.- informer.AddEventHandler(cache.ResourceEventHandlerFuncs{ + _ = informer.AddEventHandler(cache.ResourceEventHandlerFuncs{🧰 Tools
🪛 golangci-lint (1.64.8)
165-165: Error return value of
informer.AddEventHandleris not checked(errcheck)
144-152: LGTM: Effective implementation of K8s client configuration.The setup for the Kubernetes client configuration is well-implemented with proper error checking. The dynamic client creation is a good approach for interacting with custom resources.
153-158: LGTM: Correct GroupVersionResource definition for Runners CRD.The GroupVersionResource definition for the Runners CRD is correctly implemented using the appropriate Group, Version, and Resource names.
|
/terratest |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/component_test.go (1)
165-192:⚠️ Potential issueAddress potential concurrency issue and handle the return from AddEventHandler
- Multiple update events could attempt to close the channel more than once, leading to a panic. Use a check or a sync.Once guard before calling
close(stopChannel).- The static analysis points out the return from
AddEventHandleris not used. You should assign or discard it explicitly.- informer.AddEventHandler(cache.ResourceEventHandlerFuncs{ + _ = informer.AddEventHandler(cache.ResourceEventHandlerFuncs{ UpdateFunc: func(oldObj, newObj interface{}) { runner := newObj.(*unstructured.Unstructured) ... if phase == "Running" && ready == true { - close(stopChannel) + select { + case <-stopChannel: + // channel is already closed, prevent panic + return + default: + close(stopChannel) + } } }, })🧰 Tools
🪛 golangci-lint (1.64.8)
165-165: Error return value of
informer.AddEventHandleris not checked(errcheck)
🧹 Nitpick comments (1)
test/component_test.go (1)
207-209: Consider documenting the purpose of the 5-minute sleepThe long 5-minute sleep after confirming the runner is ready is without explanation. Consider adding a comment explaining why this sleep is necessary (e.g., waiting for GitHub API propagation, ensuring runner stability before verification).
- // Adding a 5 minutes sleep + // Sleep for 5 minutes to ensure the runner registration is fully propagated + // to GitHub's API and is stable before verifying with the GitHub client time.Sleep(5 * time.Minute)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/component_test.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
test/component_test.go
165-165: Error return value of informer.AddEventHandler is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (5)
test/component_test.go (5)
9-9: Appropriate imports uncommented for Kubernetes client integrationThe uncommented imports are now properly utilized for the Kubernetes client setup and resource monitoring.
Also applies to: 20-26
51-51: Namespace correctly updated to match component purposeThe namespace format has been properly updated from "external-secrets-%s" to "actions-runners-%s" to better reflect the component being tested.
144-152: Well-structured Kubernetes client configurationThe setup of the Kubernetes client configuration and dynamic client is well-implemented with proper error handling.
153-162: GroupVersionResource and informer setup looks goodThe definition of the Runners CRD GroupVersionResource and informer factory setup follows best practices.
194-204: Well-structured informer execution with timeoutThe approach of running the informer with a stop channel and timeout mechanism is appropriate. The timeout of 5 minutes is reasonable for waiting for a runner to be ready in a test environment.
|
These changes were released in v1.536.0. |
What
basiccomponent testdisabledcomponent testWhy
enabled: falsesetReferences
Summary by CodeRabbit