-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Org Specific Config in Language Server [IDE-1711][IDE-1637][IDE-1644] #1131
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?
feat: Org Specific Config in Language Server [IDE-1711][IDE-1637][IDE-1644] #1131
Conversation
…roach - Introduced LdxSyncService interface for LDX-Sync configuration management - Added in-memory caching of LDX-Sync results at Config level - Implemented parallel fetching of LDX-Sync config for workspace folders - Cache refreshes on login and workspace folder changes - Replaced OrgResolver interface with direct cache-based lookups - Removed deprecated GetBestOrgFromLdxSync function - Updated all tests to use GetOrgFromCachedLdxSync directly - Changed successful fetch logging from Info to Debug level - Added ABOUTME comments to new ldx_sync_service.go file - Documented 4 skipped integration tests in SKIPPED_TESTS_IDE-1636.md This refactor improves performance by avoiding repeated API calls and provides a cleaner architecture with better separation of concerns.
- Remove unnecessary network mocking in tests (mock at service level instead) - Use generated MockLdxSyncService instead of manual mocks - Add ResolveOrg method to LdxSyncService interface - Fix fallback tests by properly initializing engine mock - Delete SKIPPED_TESTS file as all tests now pass
…into service - Changed ResolveOrg signature to accept folderPath instead of result - Service now handles cache lookup internally, providing single entry point for LDX-Sync operations - Removed GetOrgFromCachedLdxSync from folder_handler (moved into service) - Added warning log when falling back to global org - Replaced mock with real service implementation in tests - Removed redundant tests that were testing GAF's resolver logic (already tested in GAF) - All remaining tests cover our facade responsibilities: cache lookup and fallback
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @andrewrobinsonhodges-snyk, 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 Snyk Language Server's ability to manage and apply configuration settings dynamically based on the user's organization and specific workspace folders. By integrating with LDX-Sync and introducing a robust Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
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.
Code Review
This pull request introduces a significant and well-executed refactoring to support organization-specific configurations via LDX-Sync. The introduction of the ConfigResolver and folder-aware configuration accessors provides a clear and robust way to manage settings with a defined precedence. The changes are consistently applied across different parts of the application, including scanning, diagnostics, and command handling. I've identified an opportunity to reduce code duplication in the new folder-aware product enablement checks and a TODO comment that needs clarification. Overall, this is a solid improvement to the configuration management system.
Refactored LdxSyncService to use dependency injection, enabling full mocking of external LDX-Sync API calls in tests. Added 16 test cases covering RefreshConfigFromLdxSync and ResolveOrg including success paths, error handling, caching, parallel execution, and boundary conditions.
This comment has been minimized.
This comment has been minimized.
| return FilterReasonSeverity | ||
| } | ||
| riskScoreInCLIEnabled := featureflag.UseOsTestWorkflow(folderConfig) | ||
| riskScoreInCLIEnabled := featureflag.UseOsTestWorkflowFromReader(folderConfig) |
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.
FromReader?
internal/types/ldx_sync_config.go
Outdated
| // All methods are safe for concurrent use. | ||
| type LDXSyncConfigCache struct { | ||
| mu sync.RWMutex | ||
| OrgConfigs map[string]*LDXSyncOrgConfig `json:"orgconfigs"` |
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.
use imCache (or the GAF cache) with an expiry
| }) | ||
| } | ||
|
|
||
| func TestLDXSyncConfigCache_ConcurrentAccess(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.
looks like this and the next test can be deduplicated a bit :)
| if fc == nil { | ||
| return "" | ||
| } | ||
| return fc.FolderPath |
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.
just to be safe: should we normalize here?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…nyk-ls into feat/IDE-1711_org-specific-config
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
PR Reviewer Guide 🔍
|
Description
LDX-Sync Caching
Config Resolution by Org IDE-1711
New ConfigResolver (internal/types/config_resolver.go) - Resolves effective config values with priority:
Folder-aware config accessors on Config: FilterSeverityForFolder(), IsSnykOssEnabledForFolder(), etc.
LDXSyncAdapter (internal/types/ldx_sync_adapter.go) - Converts LDX-Sync API responses to internal config format
Separation of Stored Config vs LSP Messages
Settings Validation [IDE-1644]
Key Files Changed
Checklist
make generate)make lint-fix)