Skip to content

🐛 OS Provider Segfault Fix#5480

Merged
afiune merged 4 commits intomainfrom
claytoncasey01/os-provider-segfault
May 1, 2025
Merged

🐛 OS Provider Segfault Fix#5480
afiune merged 4 commits intomainfrom
claytoncasey01/os-provider-segfault

Conversation

@claytoncasey01
Copy link
Copy Markdown
Contributor

@claytoncasey01 claytoncasey01 commented Apr 25, 2025

The os provider has some dependency on the network provider. This PR adds a dependencies array to providers that be be updated in the providers configuration. It will search through these dependencies and install any that are missing.

This is to fix: https://github.com/mondoohq/server/issues/10794

@claytoncasey01 claytoncasey01 marked this pull request as ready for review April 25, 2025 12:07
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 25, 2025

Test Results

4 234 tests  ±0   4 230 ✅ ±0   2m 12s ⏱️ -2s
  402 suites ±0       4 💤 ±0 
   30 files   ±0       0 ❌ ±0 

Results for commit 14ca7cc. ± Comparison against base commit 3cb8082.

♻️ This comment has been updated with latest results.

@vjeffrey vjeffrey requested a review from afiune April 25, 2025 18:52
Copy link
Copy Markdown
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

As we chatted, I don't think that installing the provider is the right thing to do here, instead look at the lr CLI and see if you can render the missing resources from the import that the os provider has at https://github.com/mondoohq/cnquery/blob/main/providers/os/resources/os.lr#L5 - If you look at the os.lr.manifest.yaml those resources are missing.

@claytoncasey01 claytoncasey01 force-pushed the claytoncasey01/os-provider-segfault branch from 9574572 to cd44fcd Compare April 29, 2025 15:58
@claytoncasey01
Copy link
Copy Markdown
Contributor Author

claytoncasey01 commented Apr 29, 2025

As we chatted, I don't think that installing the provider is the right thing to do here, instead look at the lr CLI and see if you can render the missing resources from the import that the os provider has at https://github.com/mondoohq/cnquery/blob/main/providers/os/resources/os.lr#L5 - If you look at the os.lr.manifest.yaml those resources are missing.

@afiune - @vjeffrey and I discussed this morning and decided for now just to go with a simple dependencies array approach. We can revisit this approach if dependencies become a normal thing.

Comment thread providers/defaults.go
Short: "an Ansible playbook",
},
},
Dependencies: []string(nil),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need all of these initializations? by default, dependencies should be empty. 🤔 what am I missing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That entire file is auto generated, so it appears to just default to []string(nil) when it doesn't have any dependencies slice in the config for that provider. Having this still allows us to range over this without explicit nil checks. We can try to force it to remove this but it seems to be the default behavior when this file is being generated.

We could add a check to only include it in the generated defaults.go if it actually has data but we would then also have to add a nil check before we attempt to loop through it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All of that said, I don't really have a preference one way or another.

Comment thread providers/providers.go Outdated
if upstreamDep != nil {
depProvider, err := Install(upstreamDep.Name, "")
if err != nil {
log.Warn().Err(err).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why installing a dependency doesn't return an error? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just a miss on my part, added.

@claytoncasey01 claytoncasey01 force-pushed the claytoncasey01/os-provider-segfault branch from f841abe to 14ca7cc Compare May 1, 2025 13:19
Copy link
Copy Markdown
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

Lets get this fixed.

@afiune afiune merged commit e2f0bf2 into main May 1, 2025
17 checks passed
@afiune afiune deleted the claytoncasey01/os-provider-segfault branch May 1, 2025 21:19
@github-actions github-actions Bot locked and limited conversation to collaborators May 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants