Skip to content
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

Table exec's should use tablehelpers.Exec #1321

Open
directionless opened this issue Aug 30, 2023 · 4 comments
Open

Table exec's should use tablehelpers.Exec #1321

directionless opened this issue Aug 30, 2023 · 4 comments
Assignees

Comments

@directionless
Copy link
Contributor

directionless commented Aug 30, 2023

We have a couple places we create tables by parsing the output from execs. These should all use our helper. This achieves several goals:

  • Uses a standard pattern
  • Enforces timeouts
  • Logs errors

We should:

  • Find all cmd.Run invocations and replace them with tablehelpers.Exec (there may be some exceptions. execwrapper and build generators for example)
  • Add a forbidigo rule to lint for them
@directionless
Copy link
Contributor Author

I think this really means pushing the dataflatten stuff to use tablehelpers.Exec

directionless added a commit to directionless/launcher that referenced this issue Feb 15, 2024
Fixes: kolide#1321

We have a nice exec helper. This should use it.
@directionless directionless changed the title Launcher should report errors from all exec tables Table exec's should use tablehelpers.Exec Apr 19, 2024
@directionless
Copy link
Contributor Author

I think this had been blocked on adding WithUid functionality to our exec helper, and that's present now. 🎉

@James-Pickett
Copy link
Contributor

If I understand correctly, the issue suggests that we lint using forbigo to ensure that code with in the /ee/tables dir only uses tableHelpers.Exec instead of directly calling Run on commands from allowedcmd. Currently golantci-lint does not support applying specific rules to specific paths in a folder. So far the only solution I have found would involve a second execution of golangci-lint that points to a different config that would only run on this specific path. I'm not sure that is worth the overhead.

@directionless
Copy link
Contributor Author

We should evaulate it in other places. Either exclude, or move to the helper, depending

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

No branches or pull requests

3 participants