feat: splunk hec token handling #803
Conversation
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
…tion in case of exception occurence
mkolasinski-splunk
force-pushed
the
feat/splunk-hec-token-handling
branch
from
97cf053 to
c123fca
Compare
mkolasinski-splunk
force-pushed
the
feat/splunk-hec-token-handling
branch
from
60b40bb to
93fdbd7
Compare
|
Semgrep found 2 Certificate verification has been explicitly disabled. This |
mkolasinski-splunk
changed the title
mkolasinski-splunk
force-pushed
the
feat/splunk-hec-token-handling
branch
from
5d8beb4 to
48cb591
Compare
mkolasinski-splunk
force-pushed
the
feat/splunk-hec-token-handling
branch
from
2f24137 to
2ffb7d7
Compare
mkolasinski-splunk
requested review from
mbruzda-splunk and
kdoroszko-splunk
pytest_splunk_addon/splunk.py
Outdated
| @@ -323,6 +324,13 @@ def pytest_addoption(parser): | |||
| help="Should execute test or not (True|False)", | |||
| default="True", | |||
| ) | |||
| group.addoption( | |||
There was a problem hiding this comment.
why did you move this line below?
There was a problem hiding this comment.
Just by mistake, restoring previous order now.
| if is_responsive_splunk(splunk_info) and is_responsive_hec( | ||
| request, splunk_info | ||
| ): | ||
| sleep(20) |
There was a problem hiding this comment.
Why 20 seconds sleeping if both conditions are okay?
There was a problem hiding this comment.
Even if both conditions are met, as far as I observed, docker container is not fully initialized yet, it performs still few steps, including cleanup of existing HEC tokens. This sleep assures that we don't perform any actions until docker container is fully spinned. I didn't figure out a better solution (I decided not to parse container logs waiting for "Ansible playbook complete" as I don't think it's needed here)
There was a problem hiding this comment.
This may lead to flaky tests, what if the CI is slower than we expect?
Why can't we improve the already existing code and add 1 more check to lambda?
There was a problem hiding this comment.
I am honestly not sure if we should proceed with this approach. I do not see a huge value in creating token dynamically, I think using the existing option is simpler and less error prone.
I would suggest just placing a deprecation warning message when default HEC token is used (if there is a way to do that), if it is not possible then put a deprecation notice into the help message for HEC token option like you already did.
|
|
||
| RESPONSIVE_SPLUNK_TIMEOUT = 300 # seconds | ||
| # Default splunk HEC token name, matching token name when token created via env variable for CI purposes |
There was a problem hiding this comment.
If we change it accidentally, it breaks the CI?
| @@ -109,8 +118,7 @@ def pytest_addoption(parser): | |||
| "--splunk-hec-token", | |||
| action="store", | |||
| dest="splunk_hec_token", | |||
| default="9b741d03-43e9-4164-908b-e09102327d22", | |||
| help='Splunk HTTP event collector token. default is "9b741d03-43e9-4164-908b-e09102327d22" If an external forwarder is used provide HEC token of forwarder.', | |||
| help="Deprecated option. Splunk HTTP event collector token. If an external forwarder is used provide HEC token of forwarder.", | |||
There was a problem hiding this comment.
Without a default value it is a breaking change?
| if is_responsive_splunk(splunk_info) and is_responsive_hec( | ||
| request, splunk_info | ||
| ): | ||
| sleep(20) |
There was a problem hiding this comment.
This may lead to flaky tests, what if the CI is slower than we expect?
Why can't we improve the already existing code and add 1 more check to lambda?
|
@mkolasinski-splunk we don't need this PR anymore? |
pytest_exception_interactin plugin.py to exit entire test suite in case ofExceptionoccurence