Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/review |
There was a problem hiding this comment.
Review: Nil Handling, Performance, and Logic
Nil Handling
1. No nil checks on any API response. Every resource file dereferences the SDK return value without checking for nil. If any Jamf SDK method returns (nil, nil), these all panic:
sso_settings.go—info.SsoEnabledcomputer_inventory.go—inventory.Resultscomputer_inventory.go:localUserAccounts()—inventory.LocalUserAccountscomputer_groups.go—groups.Resultsusers.go—users.Userspackages.go—inventory.Resultsuser_by_name.go—user.ID,user.Name, etc.version.go—info.Versionis nil-checked, butinfoitself is not
2. Nested struct dereferences in computer_inventory.go. c.General.Name, c.Hardware.Make, c.OperatingSystem.Name, c.Security.AutoLoginDisabled — if any sub-struct (General, Hardware, OperatingSystem, Security) is nil, this panics. The Jamf API can return partial records.
3. sso() returns a singular resource pointer (*mqlSsoSettings). If this ever needs to return nil (e.g., SSO not configured, access denied), it must set a.Sso.State = plugin.StateIsSet | plugin.StateIsNull before return nil, nil. See CLAUDE.md "Never return nil, nil from a singular resource accessor without setting StateIsNull first."
Logic Errors
4. ParseCLI leaks client secret into plaintext conf.Options. (provider.go:55-56):
conf.Options["client_secret"] = clientSecretThe secret already flows through conf.Credentials — storing it again in Options means it could be logged, serialized, or exposed in debug output. Remove client_id and client_secret from conf.Options.
5. ParseCLI env var fallback is tangled. (provider.go:60-74) When credentials come from env vars, the local variables are populated but conf.Options is only partially updated. Consider restructuring: resolve all sources (flags then env) first, then populate conf once at the end.
6. __id is never set in any CreateResource call. Per codebase conventions, __id must be explicitly set for caching. Without it, the runtime can't cache resources and the same API object fetched twice won't get a cache hit. Every CreateResource call needs:
"__id": llx.StringData("some-unique-stable-id"),7. Fragile id() cache keys — using names instead of unique IDs.
mqlComputerGroups.id()returnsName.Data— group names aren't guaranteed uniquemqlJamfPackages.id()returnsName.Data— package names can collidemqlJamfUsers.id()returnsName.Data— should use numeric ID (strconv.Itoa)
Use the actual unique ID field (formatted as string) for reliable caching.
8. Resource naming inconsistency in jamf.lr. Mixed prefixing and pluralization: jamfComputer, jamfPackages (plural), jamfUsers (plural), ssoSettings (no prefix), computerGroups (no prefix). Resource types should be singular and consistently named.
Performance
9. computerInventory() has no pagination. (computer_inventory.go:24) GetComputersInventory(url.Values{}) is called with empty params. The Jamf Pro inventory API is paginated — this will silently return only the first page. Customers with more computers than the default page size will get incomplete data. Must loop with page/page-size params.
10. computerInventoryCount() fetches ALL inventory records just to count them. (computer_inventory_count.go) It calls r.GetComputerInventory() which triggers the full inventory fetch, then counts the slice length. Jamf has a dedicated count endpoint — use it instead.
11. localUserAccounts() triggers N+1 API calls. Each jamfComputer.localUserAccounts call hits GetComputerInventoryByID. When iterating all computers' accounts, that's N additional API calls. Consider fetching local accounts in the initial inventory call if the SDK supports a section filter.
Update provider ID from cnquery/v9 to mql/v13 in config.go and jamf.lr. Fix scan example to use cnspec instead of mql. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tas50
left a comment
There was a problem hiding this comment.
Address the issues in the comment and let's see if we can get a testing doc for the provider in place so it's easy for folks to check this.
Also it needs tests.
Summary
Adds a new Jamf Pro provider to mql, enabling querying of Jamf-managed infrastructure via the Jamf Pro API.
Usage
Connect using OAuth2 client credentials and your Jamf Pro instance domain:
Credentials can also be provided via environment variables:
CLIENT_ID,CLIENT_SECRET,INSTANCE_DOMAIN.Example queries
New resources
jamfjamfComputerjamfComputer.localUserAccountsjamf.userByNameinitfor direct queries)jamfUsersjamfPackagesssoSettingscomputerGroups