Skip to content

feat: support per os settings from ldx-sync [IDE-1756]#1180

Open
andrewrobinsonhodges-snyk wants to merge 6 commits intorefactor/IDE-1786_folder-config-refactoringfrom
feat/IDE-1756_support-per-os-settings
Open

feat: support per os settings from ldx-sync [IDE-1756]#1180
andrewrobinsonhodges-snyk wants to merge 6 commits intorefactor/IDE-1786_folder-config-refactoringfrom
feat/IDE-1756_support-per-os-settings

Conversation

@andrewrobinsonhodges-snyk
Copy link
Copy Markdown
Contributor

@andrewrobinsonhodges-snyk andrewrobinsonhodges-snyk commented Mar 16, 2026

Description

Provide description of this PR and changes, if linked Jira ticket doesn't cover it in full.

Checklist

  • Tests added and all succeed
  • Regenerated mocks, etc. (make generate)
  • Linted (make lint-fix)
  • README.md updated, if user-facing
  • License file updated, if new 3rd-party dependency is introduced

@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 16, 2026 •
edited
Loading

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues
✅ Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

bastiandoetsch
bastiandoetsch reviewed Mar 22, 2026
View reviewed changes
internal/types/ldx_sync_adapter.go
case "linux":
suffix = "linux"
default:
suffix = "macos"
Copy link
Copy Markdown
Collaborator

@bastiandoetsch bastiandoetsch Mar 22, 2026

Choose a reason for hiding this comment

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

This should be Darwin

Copy link
Copy Markdown
Contributor Author

@andrewrobinsonhodges-snyk andrewrobinsonhodges-snyk Mar 31, 2026

Choose a reason for hiding this comment

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

Does that help? As long as it matches the LDX-Sync value it shouldn't matter.

internal/types/ldx_sync_adapter.go
// perOSSettings maps internal setting names to their base API field name.
// The API sends these as <base>_<os>, e.g. "cli_path_macos".
// Only the variant matching the current OS is accepted.
var perOSSettings = map[string]string{
Copy link
Copy Markdown
Collaborator

@bastiandoetsch bastiandoetsch Mar 22, 2026

Choose a reason for hiding this comment

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

Why make this a special case? We can just allow everything to be arch and os-specific.

Copy link
Copy Markdown
Contributor Author

@andrewrobinsonhodges-snyk andrewrobinsonhodges-snyk Mar 31, 2026

Choose a reason for hiding this comment

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

Why would we add unnecessary complexity to the API and UIs?? I don't think end users will want different scan behavior on different OSs.

internal/types/ldx_sync_adapter.go
// For per-OS settings, the current OS suffix is appended.
func GetLDXSyncKey(internalName string) string {
if baseName, ok := perOSSettings[internalName]; ok {
return baseName + "_" + osSuffix
Copy link
Copy Markdown
Collaborator

@bastiandoetsch bastiandoetsch Mar 22, 2026

Choose a reason for hiding this comment

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

I think, we're missing arch in general, which is important for the CLI path.

@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@snyk-pr-review-bot
Copy link
Copy Markdown

snyk-pr-review-bot bot commented Apr 7, 2026

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect OS Fallback 🟡 [minor]

The goosToSuffix function defaults all non-Windows and non-Linux operating systems to 'macos'. While this handles 'darwin' correctly, it would cause incorrect behavior on other Unix-like systems (e.g., FreeBSD, OpenBSD) which would attempt to use macOS-specific paths or binaries instead of potentially compatible Linux ones or failing gracefully.

 
func goosToSuffix(goos string) string {
	var suffix string
	switch goos {
	case "windows":
		suffix = "windows"
	case "linux":
		suffix = "linux"
	default:
		suffix = "macos"
	}
Potential Legacy Regression 🟡 [minor]

The PR removes base keys like cli_path and binary_base_url from ldxSyncSettingKeyMap and enforces an OS suffix. If the LDX-Sync API sends a configuration without an OS suffix (legacy format), getInternalSettingName will now return an empty string, causing the setting to be ignored by the synchronizer.

 
SettingAutomaticDownload:               "automatic_download",
SettingCliReleaseChannel:               "cli_release_channel",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bastiandoetsch bastiandoetsch bastiandoetsch left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

stacked on central config

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andrewrobinsonhodges-snyk @bastiandoetsch