-
Notifications
You must be signed in to change notification settings - Fork 229
Add comprehensive documentation for resource extension points #5004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Created extension-points folder with overview - Documented ARMResourceModifier - Documented Claimer - Documented Deleter - Documented ErrorClassifier - Documented Importer Co-authored-by: theunrepentantgeek <[email protected]>
- Documented KubernetesSecretExporter - Documented PostReconciliationChecker - Documented PreReconciliationChecker - Documented SuccessfulCreationHandler Co-authored-by: theunrepentantgeek <[email protected]>
Updated interface documentation to provide more context about: - When each extension should be used - What problems each extension solves - Key parameters and return values Enhanced comments for: - ARMResourceModifier - Claimer - Deleter - ErrorClassifier - Importer - KubernetesSecretExporter - PostReconciliationChecker - PreReconciliationChecker - SuccessfulCreationHandler Co-authored-by: theunrepentantgeek <[email protected]>
Updated the contributing index page to link to the new extension points documentation. Co-authored-by: theunrepentantgeek <[email protected]>
|
@copilot Please add documentation for |
Added comprehensive documentation for PreReconciliationOwnerChecker, a specialized variant of PreReconciliationChecker that validates only the owner's state before reconciliation without accessing the resource itself. Key aspects: - Detailed explanation of when to use vs PreReconciliationChecker - Real-world example from Kusto Database (cluster state blocking) - Comparison table showing differences from PreReconciliationChecker - Common patterns for blocking based on owner state - Enhanced code comments in the interface definition This extension is critical for resources where the owner's state can completely block access to child resources (e.g., Kusto databases inaccessible when cluster is powered off). Co-authored-by: theunrepentantgeek <[email protected]>
Added documentation for This extension is a specialized variant of The documentation includes:
|
theunrepentantgeek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, but there are a few things to tweak, @copilot. Please make the requested changes.
- Fixed comment grammar in importer.go (added "an") - Updated _index.md description to focus on compensating for resource variation - Changed "type assertion" to "type-check at the appropriate time" - Removed Claimer from extension points documentation and deleted claimer.md Claimer is not a contributor-facing extension point but an internal interface. Co-authored-by: theunrepentantgeek <[email protected]>
All requested changes completed in commit df58f62:
|
docs/hugo/content/contributing/extension-points/arm-resource-modifier.md
Show resolved
Hide resolved
|
|
||
| The Key Vault resource uses `ARMResourceModifier` to handle different creation modes based on whether a soft-deleted vault exists: | ||
|
|
||
| ```go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another place where we should maybe link to the example code rather than duplicate it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. No point in replicating large blocks of code; a direct link avoids issues with consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 5. **Test conditional paths**: Cover all branching logic | ||
| 6. **Test requeue behavior**: Verify multi-step deletions requeue correctly | ||
|
|
||
| Example test structure: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need to see an example test structure here? Seems a bit overkill for the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we'll remove that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed "Example test structure" sections from all extension point docs in commit 3264d4c.
| } | ||
| ``` | ||
|
|
||
| ### Pattern 5: Multiple Error Conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really a separate pattern this is just pattern 1 again, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Will remove this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed duplicate Pattern 5 from error-classifier.md in commit 3264d4c.
| 4. **Test error message enhancement**: Verify improved user messages | ||
| 5. **Test classification changes**: Assert correct classification results | ||
|
|
||
| Example test structure: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment, IMO overkill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Will check for, and remove, Example test structure across all the extension points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed "Example test structure" sections from all extension point docs in commit 3264d4c.
| Example test structure: | ||
|
|
||
| ```go | ||
| func TestFlexibleServersConfigurationExtension_Import(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
| } | ||
| ``` | ||
|
|
||
| Example test implementation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again seems overkill -- link to an existing test at most?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Linking to an existing test is smarter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed "Example test implementation" sections in commit 3264d4c. The Testing sections now provide concise guidance without detailed test structure examples.
|
@copilot, please reference the code review comments above and update the PR. |
- Removed weight from _index.md to allow alphabetical listing - Formatted table in _index.md for better readability - Added example for "Call next" guideline - Replaced Interface Definition in arm-resource-modifier.md with link to code - Replaced large code example with link to implementation - Added links to code for pattern implementations Co-authored-by: theunrepentantgeek <[email protected]>
- Replaced all "Interface Definition" sections with links to source code - Replaced large code examples with links to implementations - Removed "Example test structure" sections from all extension point docs - Removed duplicate Pattern 5 from error-classifier.md This addresses all remaining PR review comments about reducing duplication and linking to source code instead of copying it. Co-authored-by: theunrepentantgeek <[email protected]>
All PR review comments have been addressed in commits 3af00e3 and 3264d4c:
|
What this PR does
ASO v2 provides 9 extension interfaces for customizing resource behavior, but lacked documentation causing contributor friction. This PR adds comprehensive technical documentation covering all extension points, with links to source code to avoid duplication and ensure documentation stays synchronized with implementation.
New Documentation Structure
docs/hugo/content/contributing/extension-points/folder with overview and 9 detailed guidesExtension Points Documented
asoctl importNote: Claimer was initially included but removed as it is an internal interface, not a contributor-facing extension point.
Documentation Structure (per extension point)
Code Enhancement
Enhanced interface comments in
v2/pkg/genruntime/extensions/*.goto include usage guidance and problem statements inline.Documentation Improvements Based on Review
Example of Code Comment Enhancement
Closes #3902
How does this PR make you feel?
Checklist
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.