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

feat: add checks for the appropriate splunktaucclib version #1363

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

sgoral-splunk
Copy link
Contributor

@sgoral-splunk sgoral-splunk commented Oct 9, 2024

Issue number:ADDON-74237

Summary

Added splunktaucclib version checking.

Changes

  • modified _pip_is_lib_installed function to check if splunktaucclib is at least version 6.4
  • added new function _subprocess_run to return full stdout

User experience

The build process will be canceled if an incorrect version of the splunktaucclib library is used.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

@sgoral-splunk sgoral-splunk force-pushed the feat/add_splunktaucclib_version_check branch from 447a629 to 70bf755 Compare October 14, 2024 12:09
@sgoral-splunk sgoral-splunk marked this pull request as ready for review October 14, 2024 14:59
@sgoral-splunk sgoral-splunk requested review from a team as code owners October 14, 2024 14:59
lplonka-splunk
lplonka-splunk previously approved these changes Oct 15, 2024
Comment on lines 166 to 168
raise SplunktaucclibNotFound(
f"splunktaucclib is not found in {path_to_requirements_file}. "
f"Please add it there because this add-on has UI."
f"splunktaucclib is not found in {path_to_requirements_file} or has invalid version. "
f"Please add it there and check if it is at least version 6.4, because this add-on has UI."
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to separate this into 2 different exceptions / messages:

  • there is no splunktaucclib found but this add-ons has UI
  • splunktaucclib found but the developer needs to update it to the version we specify because of

I think it would be easier to understand why build does not actually work.

@@ -55,6 +61,24 @@ def _subprocess_call(
raise e


def _subprocess_run(
Copy link
Member

Choose a reason for hiding this comment

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

This looks very similar to _subprocess_call. I would suggest refactoring _subprocess_call into returning full response and using it everywhere.

@artemrys
Copy link
Member

I think that's a feat at the end of the day, ucc-gen build might be broken because of this feature. I think we need to highlight it and work on a reasoning behind.

@sgoral-splunk sgoral-splunk changed the title chore: split subprocess_call and subporcess_run into separate functions feat: add checks for the appropriate splunktaucclib version Oct 15, 2024
@sgoral-splunk sgoral-splunk requested a review from a team as a code owner October 16, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants