feat(http_pillar): added kwargs to handle extra option for http.query

[martintamare]

What does this PR do?

Add the hability to pass extra parameters to http_* ext_pillar
For example, add auth, or headers

What issues does this PR fix or reference?

Fixes: #36138

[MKLeb]

Hi @martintamare, would you be able to add a changelog entry and relevant tests? Also, it seems that the pre-commit hook "black" has failed, could you run that against your changes?
pre-commit run black --files <filename>

Looks good so far, but requesting a review from @garethgreenaway for a second opinion.

[github-actions]

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

[INFO] Initializing environment for https://github.com/saltstack/pip-tools-compile-impersonate.
[INFO] Initializing environment for https://github.com/asottile/pyupgrade.
[INFO] Initializing environment for https://github.com/saltstack/pre-commit-remove-import-headers.
[INFO] Initializing environment for https://github.com/s0undt3ch/salt-rewrite.
[INFO] Initializing environment for https://github.com/timothycrosley/isort.
[INFO] Initializing environment for https://github.com/timothycrosley/isort:toml.
[INFO] Initializing environment for https://github.com/psf/black.
[INFO] Initializing environment for https://github.com/asottile/blacken-docs.
[INFO] Initializing environment for https://github.com/asottile/blacken-docs:black==21.7b0.
[INFO] Initializing environment for https://github.com/PyCQA/bandit.
[INFO] Initializing environment for https://github.com/saltstack/invoke-pre-commit.
[INFO] Initializing environment for https://github.com/saltstack/invoke-pre-commit:blessings,distro,jinja2,msgpack,pyyaml.
[INFO] Initializing environment for https://github.com/saltstack/mirrors-nox.
[INFO] Initializing environment for https://github.com/saltstack/mirrors-nox:pip>=20.2.4,<21.2,setuptools<58.0.
[INFO] Installing environment for https://github.com/saltstack/invoke-pre-commit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Check Known Missing Docstrings...........................................Passed
- hook id: invoke
- duration: 1.3s

---

Thanks again!

[martintamare]

Hi @martintamare, would you be able to add a changelog entry and relevant tests? Also, it seems that the pre-commit hook "black" has failed, could you run that against your changes? pre-commit run black --files <filename>

Looks good so far, but requesting a review from @garethgreenaway for a second opinion.

I've added the changelog entry, but as no tests are present for those modules, i'm not confident to add new tests ;)

[martintamare]

Is there anyone that can review this ? 🙏

[MKLeb]

@martintamare are you able to merge in the latest master branch changes?

[martintamare]

Yes of course, doing !

[martintamare]

Done @MKLeb

[MKLeb]

I'm going to try to write tests for this and we can attempt to get this merged soon.

[martintamare]

@MKLeb I can help, but starting tests from scratch is way more work that I can provide.
If you wrote a skeleton I can work from that.
Cheers

[MKLeb]

@martintamare, it seems I don't have write access to your branch, so I'll paste the skeleton I wrote here...

This is a new file, tests/pytests/unit/pillar/test_http_json_pillar.py

import pytest
import salt.utils.json
from salt.modules import http
from salt.pillar import http_json


@pytest.fixture
def configure_loader_modules():
    return {
        http_json: {
            "__salt__": {
                "http.query": http.query,
            },
        },
        http: {
            "__opts__": {},
        },
    }


@pytest.mark.requires_network
@pytest.mark.parametrize("backend", ["requests", "tornado", "urllib2"])
def test_ext_pillar_can_take_http_query_kwargs(backend, httpserver):
    response = {
        "dict": {
            "backend": backend,
            "pillar_type": "http_json",
        },
    }
    header_dict = {"custom-backend-header": backend}

    # If the headers in header_dict are not in the request, httpserver will return an empty dictionary, so we know it will fail
    httpserver.expect_request(
        "/http_json_pillar/{}".format(backend),
        headers={"custom-backend-header": backend},
    ).respond_with_data(salt.utils.json.dumps(response), content_type="text/plain")
    url = httpserver.url_for("/http_json_pillar/{}".format(backend))

    actual = http_json.ext_pillar("test-minion-id", {}, url, header_dict=header_dict)
    assert actual == response

This is the test I wrote for pillar/http_json.py. It should be enough for that change, and all you would have to do is create a similar test in another new file tests/pytests/unit/pillar/test_http_yaml_pillar.py to test pillar/http_yaml.py. You can add other unit tests if you'd like, but this will likely be enough to get this merged in.

A few things to note here.

The pytest docs are here. They're actually pretty decent.

The special pytest fixture configure_loader_modules is how we inject functionality into the dunders normally available in salt for our unit tests since they are not naturally loaded and injected for the unit test suite.

You'll see the httpserver fixture being used here. This is from pytest-httpserver, and all it does is stand up a simple HTTP server for use in our tests to avoid reaching out to the internet. The (subpar) docs for this are here.

[martintamare]

Hi there, i'm trying to run test local but I did push.
I also see this issue #55325 which is totally related ;)

[martintamare]

Frankly, running tests on a dev platform is very hard for a non salt-dev.

I'm a venv (OK so far)
I run all the commands

python -m nox -e "pytest-3.7(coverage=False)" -- tests/pytests/unit/pillar/test_http_json_pillar.py

I have a success but with some errors.
The overhead is heavy when you are use to run a simple pytest ...

Just my frustrated opinion ;)

[MKLeb]

Frankly, running tests on a dev platform is very hard for a non salt-dev.

I'm a venv (OK so far) I run all the commands

python -m nox -e "pytest-3.7(coverage=False)" -- tests/pytests/unit/pillar/test_http_json_pillar.py

I have a success but with some errors. The overhead is heavy when you are use to run a simple pytest ...

Just my frustrated opinion ;)

I ran your test file locally and it passed. What are the errors you are getting?

Also, I know we are only testing headers being passed, but we may want to make sure we are returning yaml as the http response. I mocked it up here locally, and it looks like we can just remove the import salt.utils.json and use the following as the test...

@pytest.mark.requires_network
@pytest.mark.parametrize("backend", ["requests", "tornado", "urllib2"])
def test_ext_pillar_can_take_http_query_kwargs(backend, httpserver):
    response = """
    dict:
      backend: {}
      pillar_type: http_yaml
    """.format(backend)

    expected = {
        "dict": {
            "backend": backend,
            "pillar_type": "http_yaml",
        },
    }

    header_dict = {"custom-backend-header": backend}

    # If the headers in header_dict are not in the request, httpserver will return an empty dictionary, so we know it will fail
    httpserver.expect_request(
        "/http_yaml_pillar/{}".format(backend),
        headers={"custom-backend-header": backend},
    ).respond_with_data(response, content_type="text/plain")
    url = httpserver.url_for("/http_yaml_pillar/{}".format(backend))

    actual = http_yaml.ext_pillar("test-minion-id", {}, url, header_dict=header_dict)
    assert actual == expected

Also, if you remove the .. autofunction lines in both of your modified files that should stop the docs check from failing.

[MKLeb]

Closing this because I ported it to #62788 to make the necessary fixes