Resolve meraki webhook fail when none defined

[mystery-rabbit]

Resolves issue #433

Original Issue report concerned meraki_webhook failing when a query returns a list with no entries.

PR adds test coverage for the observed issue, and makes sanitize_no_log_values work with lists of any length.

sanity and integration tests passed.

[y0rune]

Hello!

The all tests passed in my dev env.
The ansible-lint was not passed due to some kind of warnings. I will fix it in the #361.

Please add the changelogs/fragments/xyz.yml as bugfix .

Example file of changelogs/fragments: https://github.com/CiscoDevNet/ansible-meraki/pull/432/files#diff-7c42c7e2a287182c3da289d14dddb44f293da1bf306b0112cd93cebba21835fb

--
Marcin

[mystery-rabbit]

@y0rune - added, i think, so please review.

• for future use - is there any docs/guide to the fragments files? - it's not mentioned in contributing.md.

[kbreit]

https://github.com/ansible-community/antsibull-changelog/blob/main/docs/changelogs.rst

[kbreit]

Why a while loop instead of a for loop here?

[mystery-rabbit]

Ah. Your comment has made me re-look at this. There is still an issue here so it's not ready for merge.

So that is where i started - I've just tried this again to check - and I think i've been miss-interpreted an error i saw..

with ...

        for i in meraki.result['data']:
            i[
                "shared_secret"
            ] = "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER"

... I get the "Query one webhook" test failing with TypeError: 'str' object does not support item assignment. I took that to be an issue with the indirect assignment afforded by the for loop variable and so used a while loop instead with direct assignment.

I've miss-understood the meraki.result behaviour - assuming data is always a list - be it empty or with multiple entries. With a query naming a web hook, I'm guessing now data is a dict, not a single entry list pointing to a dict.

I'll look at this again.

[mystery-rabbit]

I've re-pushed this - ready for re-review.

[kbreit]

Overall this looks good but should there be a new test looking for the shared_secret value?

[mystery-rabbit]

We can certainly use the opportunity to call out the expected behaviour via the test cases yes. that's probably just the de-commenting of the idempotency tests and tightening the assert statements.

(.venv) vagrant@terra01:~/ansible_collections/cisco/meraki$ grep "\- name" tests/integration/targets/meraki_webhook/tasks/tests.yml 
  - name: Test an API key is provided
  - name: Create test network
  - name: Query for any webhooks expecting None
  - name: Create webhook with check mode
  - name: Create webhook
  - name: Query all webhooks expecting 1
  - name: Query one webhook
  - name: Query one webhook with id
  - name: Update webhook with check mode
  # - name: Update webhook
  # - name: Update webhook with idempotency
  # - name: Update webhook with id
  - name: Create webhook payload template for a webhook
  - name: Debug payload_template
  - name: Create webhook with a payload template
  - name: Delete webhook with payload template
  - name: Create test webhook
  - name: Get webhook status
  - name: Query all webhooks
  - name: Delete webhook invalid webhook
  - name: Delete webhook in check mode
  - name: Delete webhook
  - name: Delete webhook payload template for a webhook
  - name: Delete test network

[mystery-rabbit]

@kbreit

• I added a test to my local branch testing that a seemingly idempotent change is always going to return "changed" and no shared_secret. passed.

• I then added a test that did a change without shared_secret set - expecting changed='false' - and that's picked up another issue.

API behaviour - if the shared secret is not set in the request, then the API will still return 200 OK - it does not touch the secret - only updating any changed params. (verified through GUI/Postman)

module behaviour - if the shared secret param is not set, the module will set shared secret to null, and send that - API will return 400 Error. To my mind, it should not be setting the param at all.

I can see how to resolve that - any issues with me adding that to the PR?

[kbreit]

Lets merge this one and add the other fixes in a new PR.