salt icon indicating copy to clipboard operation
salt copied to clipboard

Adds __env__ substitution to ext_pillar.stack

Open morgana2313 opened this issue 3 years ago • 13 comments

What does this PR do?

  • Adds __env__substitution to ext_pillar.stack; followup of #61531
  • Improved exception handling for stacked template (jinja) template rendering and yaml parsing in ext_pillar.stack

New Behavior

adds __env__ to stacked pillar config

    ext_pillar:
      - stack:
          opts:pillarenv:
            dev: /path/to/dev/stack.cfg
            __env__: /path/to/__env__/stack.cfg

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

  • [x] Docs
  • [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
  • [x] Tests written/updated

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@nicholasmhughes: I think this code is working ok, do you agree?

morgana2313 avatar Aug 30 '22 18:08 morgana2313

can you rebase your branch against master from the start? it looks like you are pulling in changes from all over. which makes this almost impossible to review.

whytewolf avatar Aug 30 '22 18:08 whytewolf

can you rebase your branch against master from the start? it looks like you are pulling in changes from all over. which makes this almost impossible to review.

Done.

morgana2313 avatar Aug 31 '22 07:08 morgana2313

@whytewolf: I have no idea how to proceed. The pr-opensuse-15-x86_64-py3-pytest says: Failing after 71m — Run Tests (Fast): error in 'sh' step but I don't understand how that may be causes by these changes. Can you please advise?

morgana2313 avatar Sep 01 '22 09:09 morgana2313

@whytewolf, @nicholasmhughes: the tests give some errors that I think are unrelated to my changes. I don't know how to proceed, please advise.

morgana2313 avatar Sep 05 '22 07:09 morgana2313

re-run pr-windows-2019-x64-py3-pytest

nicholasmhughes avatar Sep 06 '22 04:09 nicholasmhughes

re-run pr-windows-2019-x64-py3-pytest

I've triggered a rerun with a new push, but I don't see how I can trigger a rerun otherwise. I also removed the [WIP] tag in the subject line and made updates to /docs/man/salt.7 and /changelog

As far as I can see there are no ext_pillar.stack unit tests that I can update for this __env__ behaviour.

morgana2313 avatar Sep 06 '22 09:09 morgana2313

re-run pr-windows-2019-x64-py3-pytest

I've triggered a rerun with a new push, but I don't see how I can trigger a rerun otherwise. I also removed the [WIP] tag in the subject line and made updates to /docs/man/salt.7 and /changelog

As far as I can see there are no ext_pillar.stack unit tests that I can update for this __env__ behaviour.

there may not be an existing test, that does happen. If possible would you be able to make a new test for the functionality?

whytewolf avatar Sep 06 '22 19:09 whytewolf

there may not be an existing test, that does happen. If possible would you be able to make a new test for the functionality?

@whytewolf: I've added a stacked pillar unit test, testing the old and new functionality.

morgana2313 avatar Sep 15 '22 11:09 morgana2313

@whytewolf:

The friendly PR-robot tells me

The function 'ext_pillar' on 'salt/pillar/stack.py' does not have a docstring

That's true, but the docstring is in the top of the file. Should I move the docstring, add a dummy reference or leave this as it is?

I've filed bug report #62693 that I discovered working on this PR.

I consider this PR ready for merging in master.

morgana2313 avatar Sep 16 '22 09:09 morgana2313

@whytewolf:

The friendly PR-robot tells me

The function 'ext_pillar' on 'salt/pillar/stack.py' does not have a docstring

That's true, but the docstring is in the top of the file. Should I move the docstring, add a dummy reference or leave this as it is?

I've filed bug report #62693 that I discovered working on this PR.

I consider this PR ready for merging in master.

take inspiration for the other ext_pillar modules. the docstring for the ext_pillar function should be short and to the point of what the function does. the one at the top that covers the enter functionality should be left alone. as that is the main point of the entire module.

the one for ext_pillar should be something like Execute queries against MySQL, merge and return as a dict

whytewolf avatar Sep 16 '22 09:09 whytewolf

@whytewolf: ok I've added the docstring, and all checks pass. So now I need a review. Can you do the code review?

Regards, Jasper

morgana2313 avatar Sep 19 '22 08:09 morgana2313

looks like there are still changes to the man pages

Oh, my bad. Fixed.

morgana2313 avatar Sep 23 '22 07:09 morgana2313

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)
Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- exit code: 1

/home/runner/.cache/pre-commit/repo7yawj1iq/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:33: UserWarning: Setuptools is replacing distutils. warnings.warn("Setuptools is replacing distutils.") The function 'ext_pillar' on 'salt/pillar/stack.py' does not have a docstring Found 1 errors


Thanks again!

github-actions[bot] avatar Sep 23 '22 07:09 github-actions[bot]

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)
Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- exit code: 1

/home/runner/.cache/pre-commit/repo7yawj1iq/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:33: UserWarning: Setuptools is replacing distutils. warnings.warn("Setuptools is replacing distutils.") The function 'ext_pillar' on 'salt/pillar/stack.py' does not have a docstring Found 1 errors


Thanks again!

github-actions[bot] avatar Sep 23 '22 07:09 github-actions[bot]

Congratulations on your first PR being merged! :tada:

welcome[bot] avatar Sep 26 '22 18:09 welcome[bot]