refactor: Added pattern checker. Handled deletion errors. Added test cases.#199
refactor: Added pattern checker. Handled deletion errors. Added test cases.#199rmandal-redhat wants to merge 53 commits into
Conversation
Summary of ChangesHello @rmandal-redhat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and error handling of the Group controller. It introduces a pattern-based configurability check for Group resources, ensuring that reconciliation only proceeds for groups with valid backend configurations. Furthermore, the deletion finalizer has been refactored to perform best-effort cleanup across all backends, gracefully handling individual deletion failures. New utility functions and comprehensive test cases have also been added to support these improvements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Patterns now checked, Deletion errors handled, Code stands strong and clear. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request introduces a new pattern checker for group configurability, enhancing the operator's ability to handle groups that do not conform to defined naming patterns. It also significantly improves the robustness of the finalization process by implementing more graceful error handling during team and cache deletion, allowing cleanup to proceed even if some operations fail. Several new test cases have been added to validate these new behaviors, and minor but important updates were made to .gitignore and Makefile for better security and build consistency. Overall, these changes contribute to a more resilient and predictable operator.
|
Signed-off-by: Rupamanjuri Mandal rmandal@redhat.com |
- Add missing apiSecret to backend connection config in first test - Remove mock expectations for LDAP calls when group is not configurable - Update test assertions to verify successful reconciliation - Both tests now pass and reconciliation completes without errors Note: Test fixtures use dummy secrets which are acceptable for testing Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: vinamra28 <vinjain@redhat.com> Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: Pujathacker2210 <pthacker@redhat.com> Signed-off-by: rmandal <rmandal@redhat.com>
…plication Signed-off-by: Pujathacker2210 <pthacker@redhat.com> Signed-off-by: rmandal <rmandal@redhat.com>
… for preloadcache function Signed-off-by: Pujathacker2210 <pthacker@redhat.com> Signed-off-by: rmandal <rmandal@redhat.com>
…envs Signed-off-by: Pujathacker2210 <pthacker@redhat.com> Signed-off-by: rmandal <rmandal@redhat.com>
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 6.1.0 to 6.2.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@4dc6199...7a3fe6c) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: 6.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: Sunando Bhattacharya <subhatta@redhat.com> Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: rmandal <rmandal@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable refactorings. The addition of the isGroupConfigurable check is a great feature that prevents unnecessary reconciliation on misconfigured Group resources, improving efficiency and clarity. The finalizer logic in deleteBackendsTeam has been made more robust by adopting a best-effort cleanup strategy, which is excellent for deletion workflows. The new GetTransformedGroupNameOrFallback utility and its corresponding tests are well-implemented. I have two suggestions: one to align the finalizer's behavior with its logging to ensure deletion is not blocked unnecessarily, and another to enhance test coverage for the new status conditions.
maxkashyap41
left a comment
There was a problem hiding this comment.
Added few edge cases scenarios where the reconciler might fail again for transformed group fallback use case.
Signed-off-by: Madhurjya Das <madhurjyakumardas@gmail.com> Independent reconciliation of Group Params Signed-off-by: Madhurjya Das <madhurjyakumardas@gmail.com> Independent reconciliation of Group Params v2 Signed-off-by: Madhurjya Das <madhurjyakumardas@gmail.com>
- Added exclusion list support for user offboarding job - Implemented security checks to skip null or empty userKey values - Optimized exclusion list lookup using map data structure for O(1) performance - Added support for loading exclusion list from local files or HTTP URLs - Added metrics for offboarding_skipped_exclusion_list and offboarding_validation_failures Signed-off-by: Sunando Bhattacharya <subhatta@redhat.com>
Bumps [github.com/redis/go-redis/v9](https://github.com/redis/go-redis) from 9.17.2 to 9.17.3. - [Release notes](https://github.com/redis/go-redis/releases) - [Changelog](https://github.com/redis/go-redis/blob/v9.17.3/RELEASE-NOTES.md) - [Commits](redis/go-redis@v9.17.2...v9.17.3) --- updated-dependencies: - dependency-name: github.com/redis/go-redis/v9 dependency-version: 9.17.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.14.0 to 2.14.2. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@20cf305...5ef0c07) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-version: 2.14.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/alicebob/miniredis/v2](https://github.com/alicebob/miniredis) from 2.34.0 to 2.36.1. - [Release notes](https://github.com/alicebob/miniredis/releases) - [Changelog](https://github.com/alicebob/miniredis/blob/master/CHANGELOG.md) - [Commits](alicebob/miniredis@v2.34.0...v2.36.1) --- updated-dependencies: - dependency-name: github.com/alicebob/miniredis/v2 dependency-version: 2.36.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [k8s.io/client-go](https://github.com/kubernetes/client-go) from 0.34.3 to 0.34.4. - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.34.3...v0.34.4) --- updated-dependencies: - dependency-name: k8s.io/client-go dependency-version: 0.34.4 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.27.5 to 2.28.1. - [Release notes](https://github.com/onsi/ginkgo/releases) - [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md) - [Commits](onsi/ginkgo@v2.27.5...v2.28.1) --- updated-dependencies: - dependency-name: github.com/onsi/ginkgo/v2 dependency-version: 2.28.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/dependency-review-action](https://github.com/actions/dependency-review-action) from 4.8.2 to 4.8.3. - [Release notes](https://github.com/actions/dependency-review-action/releases) - [Commits](actions/dependency-review-action@3c4e3dc...05fe457) --- updated-dependencies: - dependency-name: actions/dependency-review-action dependency-version: 4.8.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.14.2 to 2.15.0. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@5ef0c07...a90bcbc) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-version: 2.15.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 6.2.0 to 6.3.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@7a3fe6c...4b73464) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: 6.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [go.opentelemetry.io/otel/sdk](https://github.com/open-telemetry/opentelemetry-go) from 1.34.0 to 1.40.0. - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-go@v1.34.0...v1.40.0) --- updated-dependencies: - dependency-name: go.opentelemetry.io/otel/sdk dependency-version: 1.40.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.15.1 to 2.16.0. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@58077d3...fa2e9d6) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-version: 2.16.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…licts) Made-with: Cursor # Conflicts: # .gitignore # appconfig/default.yaml # internal/controller/group_controller.go # pkg/utils/utils.go # pkg/utils/utils_test.go
standardize first/last names for backend --------- Signed-off-by: tikandeabhay <38690658+tikandeabhay@users.noreply.github.com> Co-authored-by: Abhay Tikande <atikande@redhat.com>
feat: Add support for ldap query based groups Signed-off-by: Bibhas <github@bibhasdn.com>
Signed-off-by: Bibhas Debnath <bdebnath@redhat.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.72.1 to 1.79.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.72.1...v1.79.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.79.3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/goccy/go-yaml](https://github.com/goccy/go-yaml) from 1.18.0 to 1.19.2. - [Release notes](https://github.com/goccy/go-yaml/releases) - [Changelog](https://github.com/goccy/go-yaml/blob/master/CHANGELOG.md) - [Commits](goccy/go-yaml@v1.18.0...v1.19.2) --- updated-dependencies: - dependency-name: github.com/goccy/go-yaml dependency-version: 1.19.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
- Add missing apiSecret to backend connection config in first test - Remove mock expectations for LDAP calls when group is not configurable - Update test assertions to verify successful reconciliation - Both tests now pass and reconciliation completes without errors Note: Test fixtures use dummy secrets which are acceptable for testing Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: rmandal <rmandal@redhat.com>
Signed-off-by: Sunando Bhattacharya <subhatta@redhat.com> Signed-off-by: rmandal <rmandal@redhat.com>
Bumps [k8s.io/apimachinery](https://github.com/kubernetes/apimachinery) from 0.34.4 to 0.34.6. - [Commits](kubernetes/apimachinery@v0.34.4...v0.34.6) --- updated-dependencies: - dependency-name: k8s.io/apimachinery dependency-version: 0.34.6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/redis/go-redis/extra/redisotel/v9](https://github.com/redis/go-redis) from 9.17.2 to 9.18.0. - [Release notes](https://github.com/redis/go-redis/releases) - [Changelog](https://github.com/redis/go-redis/blob/master/RELEASE-NOTES.md) - [Commits](redis/go-redis@v9.17.2...v9.18.0) --- updated-dependencies: - dependency-name: github.com/redis/go-redis/extra/redisotel/v9 dependency-version: 9.18.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/gojek/heimdall/v7](https://github.com/gojek/heimdall) from 7.0.3 to 7.1.0. - [Release notes](https://github.com/gojek/heimdall/releases) - [Changelog](https://github.com/gojek/heimdall/blob/master/CHANGELOG.md) - [Commits](gojek/heimdall@v7.0.3...v7.1.0) --- updated-dependencies: - dependency-name: github.com/gojek/heimdall/v7 dependency-version: 7.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/alicebob/miniredis/v2](https://github.com/alicebob/miniredis) from 2.36.1 to 2.37.0. - [Release notes](https://github.com/alicebob/miniredis/releases) - [Changelog](https://github.com/alicebob/miniredis/blob/master/CHANGELOG.md) - [Commits](alicebob/miniredis@v2.36.1...v2.37.0) --- updated-dependencies: - dependency-name: github.com/alicebob/miniredis/v2 dependency-version: 2.37.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [k8s.io/client-go](https://github.com/kubernetes/client-go) from 0.34.4 to 0.34.6. - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.34.4...v0.34.6) --- updated-dependencies: - dependency-name: k8s.io/client-go dependency-version: 0.34.6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/go-ldap/ldap/v3](https://github.com/go-ldap/ldap) from 3.4.12 to 3.4.13. - [Release notes](https://github.com/go-ldap/ldap/releases) - [Commits](go-ldap/ldap@v3.4.12...v3.4.13) --- updated-dependencies: - dependency-name: github.com/go-ldap/ldap/v3 dependency-version: 3.4.13 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
There are cases where a user can have `-` in their username for example `usernaut-user`. Snowflake doesn't supports `-` in the name field and in order to support such users, we should populate the `login_name` field which can contain `-`. Also, updating the other missing fields. Signed-off-by: vinamra28 <vinjain@redhat.com>
…licts) Made-with: Cursor # Conflicts: # .gitignore # appconfig/default.yaml # internal/controller/group_controller.go # pkg/utils/utils.go # pkg/utils/utils_test.go
A merge introduced two identical function definitions and two TestGetTransformedGroupNameOrFallback blocks, which broke compilation. Keep a single implementation and merge fallback test cases. Signed-off-by: Rupamanjuri Mandal Das <rmandal@redhat.com>
TestLoadImageToKindClusterWithName executed 'kind load docker-image', which can hang until the test timeout when Docker/Kind is missing or misconfigured. Assert argv via kindLoadDockerImageCommand instead; e2e still runs LoadImageToKindClusterWithName. Signed-off-by: Rupamanjuri Mandal Das <rmandal@redhat.com> Made-with: Cursor
…licts) Made-with: Cursor # Conflicts: # .gitignore # appconfig/default.yaml # internal/controller/group_controller.go # pkg/utils/utils.go # pkg/utils/utils_test.go
…licts) Made-with: Cursor # Conflicts: # .gitignore # appconfig/default.yaml # internal/controller/group_controller.go # pkg/utils/utils.go # pkg/utils/utils_test.go Signed-off-by: Rupamanjuri Mandal Das <rmandal@redhat.com>
Signed-off-by: Rupamanjuri Mandal Das <rmandal@redhat.com>
Signed-off-by: Rupamanjuri Mandal Das <rmandal@redhat.com>
…pcontroller_test.go Signed-off-by: Rupamanjuri Mandal Das <rmandal@redhat.com>
Signed-off-by: Rupamanjuri Mandal Das <rmandal@redhat.com>
Changes
📝 Description
What changed?
Why is this change needed?
Dependencies
🧪 Testing
Test Coverage
Performance Impact
🚀 Deployment
Deploy Steps
Prerequisites
Post-Deployment Monitoring
Rollback Plan
Details:
⚙️ Configuration Changes
✅ Developer Checklist