Ensure netscaler_nitro_request get_filtered uses all filters, fixes #48

[JonasVerhofste]

SUMMARY

Loop over all filters and combine them with &. Before this, only the first filter was used.
Won't do the urlencode in the python code for now, as that would create backwards incompatibility with existing plays ('double' urlencode, see the example in #48)

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

netscaler_nitro_request

[felixfontein]

Please add a changelog fragment.

[JonasVerhofste]

@felixfontein Done :)

[JonasVerhofste]

Also, like I said: the cleanest would be to do the percent-encoding of the parameters in the module, but that's gonna create a huge backwards incompatibility, as many plays with filters like #48 probably exist.

Not sure how you guys would tackle such a change?

[felixfontein]

That's indeed a good question. I guess the best approach would be to add a new optional boolean option, filter_percent_encode. If it is explicitly set to yes or no, it will do as currently set. Documentation should state that now the default is no, but it will change to yes in Ansible 2.14 (the version will be replaced by a collection version once we decided on how to map Ansible version deprecations to collection version deprecations). Also, add code which checks that in case the option is not specified (i.e. the value is None), whether any filter would change when being percent encoded, and if it does (because it already is percent encoding, or because it needs percent encoding), print a deprecation warning that it won't get percent encoded right now by the module, but it will from Ansible 2.14 on - and if the user wants percent encoding done by the module, she can explicitly enable or disable it with the new option.

That would keep current playbooks working, fix the problem in the future, and tell people who rely on no automatic percent encoding that they have to do something.

I would definitely do that in a separate PR though :)

[felixfontein]

LGTM

[felixfontein]

@JonasVerhofste just to make sure: I assume you checked the latest version of this PR with a real service instance?

[JonasVerhofste]

I would definitely do that in a separate PR though :)

Oh, definitely! I'll make a separate issue for it as well. Thanks for the advice!
I'll see if I can implement this one of coming weeks.

I assume you checked the latest version of this PR with a real service instance?

@felixfontein I just did, works like a charm!

[felixfontein]

@JonasVerhofste thanks for fixing this!