splunk / contentctl

Splunk Content Control Tool
Apache License 2.0
80 stars 20 forks source link

Handling when a user does not answer one of the questions #189

Closed yaleman closed 1 month ago

yaleman commented 1 month ago

If you ran contentctl new and hit ctrl-c it'd end up throwing a slightly confusing error:

$ contentctl new
? enter detection name Powershell Encoded Command

Cancelled by user

Verbose error logging is ENABLED.
The entire stack trace has been provided below (please include it if filing a bug report):

Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/contentctl/contentctl.py", line 205, in main
    new_func(config)
  File "/usr/local/lib/python3.12/site-packages/contentctl/contentctl.py", line 93, in new_func
    NewContent().execute(config)
  File "/usr/local/lib/python3.12/site-packages/contentctl/actions/new_content.py", line 97, in execute
    content_dict = self.buildDetection()
                   ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/contentctl/actions/new_content.py", line 21, in buildDetection
    answers['name'] = answers['detection_name']
                      ~~~~~~~^^^^^^^^^^^^^^^^^^
KeyError: 'detection_name'
pyth0n1c commented 1 month ago

Hi @yaleman, thank you for the PR! I have looked at the questionary codebase, and when a keyboard interrupt is captured during a questionary.prompt() call, there is an inconsistency between the documentation (should return None) and codebase for questionary (actually returns {}). I have opened an issue here: https://github.com/tmbo/questionary/issues/385

I would propose that instead, we do two things:

  1. Use a more descriptive keyboard interrupt message, which is supported by questionary, such as: answers = questionary.prompt(questions,kbi_msg="User did not answer all of the prompt questions. Exiting...")
  2. Check the return value or the questionary.prompt() and, if it is None (or {}, depending on the response from the questionary team) we raise an Exception which is then handled properly by the contentctl.py's catch-all exception handler.
yaleman commented 1 month ago

That'd work as well 😄 Apologies for the epic reformat, my defaults in VS code are a little different, can clean that up when I go back to make changes.

We don't even have to check for {} / None, it can just be if not answers which does the same.

The catch-nothing exception sounds sensible, if I should create a new one please suggest a name, or one that will be handled (because the KeyError wasn't 😄 )

yaleman commented 1 month ago

Cleaned up the commit and made the handling a little cleaner (turns out, vs-code has a "save without formatting option").

pyth0n1c commented 1 month ago

I am still waiting on some feedback on the Issue I created in the questionary repo: https://github.com/splunk/contentctl/pull/189#:~:text=tmbo/questionary%23385

I have decided we will merge this in, as your updated changes handle both cases {} and None. I will keep an eye on the linked questionary issue above.

I have updated this to go into v4.2 which we expect to release soon rather than directly into main.