fix(controller): correctly use sentinel run job check to run targeted integration tests config#688
Open
fix(controller): correctly use sentinel run job check to run targeted integration tests config#688
Conversation
Add info-level logging to track the building and processing of integration tests within sentinel checks.
Contributor
Greptile SummaryThis PR fixes the sentinel run controller so that only the configuration matching the specific named check ( Several issues flagged in previous review rounds have been resolved in this iteration:
Two remaining items worth addressing:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| pkg/sentinel-harness/controller/controller.go | Core refactoring: adds getIntegrationTestConfiguration, buildGotestsumRunArgs, and createTestCasesFile. Previous issues (double-call of buildGotestsumRunArgs, nil-dereference on SentinelRun) are now fixed. The removal of --test.parallel 1 as a default changes behaviour slightly but is intentional. |
| pkg/sentinel-harness/controller/controller_test.go | New test file covering buildGotestsumRunArgs. The --rerun-fails=4 assertion now correctly uses mustContainArg with the =-joined form (addressing the previously flagged issue). One minor gap: the assertion helpers do exact string matching, so bare --rerun-fails and --rerun-fails=N are treated as different strings — any variant not explicitly checked could slip through. |
| pkg/sentinel-harness/controller/controller_types.go | Removed the Name field from TestCase struct, consistent with the switch from a keyed list to a single per-check object. |
| pkg/sentinel-harness/controller/templating.go | buildBindings signature simplified to accept only a cluster fragment; error return removed since it can never error. |
| terratest/integration_test.go | Refactored to use a single TestCase instead of a slice. loadIntegrationTestCases now fails fast when the env var is unset. The type-specific sub-config nil-checks (e.g. coredns/lb) are still present inside each run* helper. One minor style issue in the error handling pattern. |
| terratest/types/testcase.go | Mirrors the controller-side TestCase change: Name field removed, now a flat container for Configurations and Defaults. |
| go.mod | Bumps Go toolchain to 1.26.1 and upgrades several dependencies including k8s.io/api to 0.35.x, DataDog tracer to 2.6.0, and golangci-lint to 2.11.3. One minor note: go.mod uses go 1.26.1 while some toolchain entries may still reference the old version in go.sum. |
Last reviewed commit: 5ee611c
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Add unit tests for `buildGotestsumRunArgs` in `controller_test.go`. - Remove unused `Name` field from `TestCase` struct. - Update module dependencies in `go.sum`.
- Remove unused `Name` field from `TestCase` struct. - Refactor integration tests for better clarity and maintainability by reducing nested loops and improving variable naming. - Implement early validation for test case configurations. - Add support to verify `--packages=./...` args in controller unit tests.
floreks
changed the title
- Bump Go version from 1.25.7 to 1.26.1 across multiple Dockerfiles. - Update all relevant build and workflow configurations to use the new Go version.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Bump golangci-lint version from v2.8.0 to v2.11.3 in GitHub Actions CI configuration and go.mod. - Upgrade multiple indirect dependencies to their latest versions in go.mod for improved stability and performance.
- Bump golangci-lint version from v2.8.0 to v2.11.3 in GitHub Actions CI configuration and go.mod. - Upgrade multiple indirect dependencies to their latest versions in go.mod for improved stability and performance.
- Add `.PHONY: build` target to Makefile for building all binaries. - Upgrade multiple dependencies to their latest versions in go.mod and go.sum for improved stability and performance.
…luralsh/deployment-operator into fix/sentinel-integration-test-handling
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
- Upgrade k8s.io/api, k8s.io/client-go, k8s.io/apiserver, and k8s.io/component-base to v0.35.2. - Add k8s.io/klog v1.0.0 for improved logging capabilities. - Use `replace` directive for k8s.io/cloud-provider v0.35.2.
- Upgrade `k8s.io/apiextensions-apiserver` to v0.35.2. - Add indirect dependencies: `github.com/NYTimes/gziphandler`, `gopkg.in/natefinch/lumberjack.v2`, `github.com/coreos/go-semver`, and more for enhanced functionalities and performance.
…TAG` - Remove unused dependencies from go.mod and go.sum for a cleaner configuration. - Downgrade GO_FIPS_IMAGE_TAG from 1.26.1
- Refactor rerun-failures logic to correctly handle rerun counts using `lo.FromPtr`. - Simplify the condition for appending rerun-failures argument based on rerun count.
…inel fragment checks` - Add validation for test case configurations to ensure they have names and types. - Modify running of gotestsum with corrected args in controller. - Add check for `SentinelRun` presence in test configuration fragments.
Member
Author
- Change argument check from `mustContainArgPair` to `mustContainArg` for `--rerun-fails=4`.
Member
Author
|
@greptileai fixed tests, PTAL |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactoring and feature improvements:
sentinelRunController: Added a new methodgetIntegrationTestConfigurationto cleanly fetch the relevant configuration for a given check, and updatedrunTeststo use this method. This makes the code more robust and easier to maintain.buildGotestsumRunArgsutility: Centralized the construction ofgotestsumcommand-line arguments, supporting configuration overrides (such as rerun-fails, parallelism, and process count) and added comprehensive unit tests for this logic incontroller_test.go.createTestCasesFilefunction that builds a single test case based on the selected configuration, improving clarity and correctness.Utility updates:
buildBindingsto accept only the cluster fragment (instead of the entire job fragment), simplifying the interface and usage.Integration test runner improvements:
terratest/integration_test.goto use the new test case structure, simplifying the execution logic and making it easier to add new test types.Test Plan
Test environment: https://console.plrl-dev-aws.onplural.sh/
Checklist