Skip to content

Comments

feat: improve the override failure handling and message#1064

Closed
ryanzhang-oss wants to merge 2 commits intoAzure:mainfrom
ryanzhang-oss:improve-override-error
Closed

feat: improve the override failure handling and message#1064
ryanzhang-oss wants to merge 2 commits intoAzure:mainfrom
ryanzhang-oss:improve-override-error

Conversation

@ryanzhang-oss
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss commented Mar 2, 2025

Description of your changes

  1. Be clear on what resource is the override apply failed
  2. Add e2e on override key success and failure cases
  3. Add e2e on cross namespace envelope failure cases
  4. Add e2e on clusterAPI object

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@ryanzhang-oss ryanzhang-oss marked this pull request as draft March 2, 2025 06:23
@Arvindthiru
Copy link
Contributor

We need to update this doc https://github.com/Azure/fleet/blob/main/docs/howtos/envelope-object.md#example-crp-status-where-resource-within-an-envelope-object-failed-to-apply

@ryanzhang-oss ryanzhang-oss changed the title fix: block cross namespace envelop obj feat: improve the override failure handling and message Mar 12, 2025
@ryanzhang-oss ryanzhang-oss force-pushed the improve-override-error branch 4 times, most recently from d06d219 to b29f820 Compare March 14, 2025 01:00
@ryanzhang-oss ryanzhang-oss force-pushed the improve-override-error branch from b29f820 to df62bbc Compare March 14, 2025 22:13
@ryanzhang-oss ryanzhang-oss requested a review from Copilot March 21, 2025 01:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the override failure handling and messaging while adding end-to-end tests to cover various override key scenarios and cross-namespace envelope cases.

  • Updated error messages and logging in override rule application for both cluster and resource scopes.
  • Added new e2e test contexts to validate proper behavior when using templated override rules and handling non‐existent cluster label keys.
  • Minor refactors in controller and test files to improve clarity and consistency in resource extraction and error reporting.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/e2e/placement_ro_test.go Added Placement assignment to ResourceOverride specs in tests.
pkg/controllers/workgenerator/override.go Updated error messages and improved error handling in overrides.
pkg/controllers/workgenerator/controller.go Simplified error messaging and renamed variables for clarity.
test/e2e/actuals_test.go Improved error messages in work namespace removal test.
pkg/controllers/workgenerator/override_test.go Minor improvements in error conversion in override tests.
Comments suppressed due to low confidence (2)

pkg/controllers/workgenerator/override.go:270

  • Verify that constructing fullVariable via fmt.Sprintf correctly matches the intended substring in the input. The removal of dynamic index usage might cause incorrect replacements if multiple occurrences exist.
fullVariable := fmt.Sprintf("%s%s}", placementv1alpha1.OverrideClusterLabelKeyVariablePrefix, keyName)

pkg/controllers/workgenerator/controller.go:191

  • [nitpick] Confirm that 'errorMessage' contains sufficient contextual details for debugging, as the previous formatted message provided additional clarity.
Message:            errorMessage,

@michaelawyu
Copy link
Contributor

Hi Ryan! I am closing this PR as part of the CNCF repo migration process; please consider moving (re-creating) this PR in the new repo once the sync PR is merged. If there's any question/concern, please let me know. Thanks 🙏

@ryanzhang-oss ryanzhang-oss deleted the improve-override-error branch April 22, 2025 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants