add kwargs to be passed from grains.setval to saltutil.refresh_grains

[jtraub91]

What does this PR do?

Allows kwargs to be passed thru to grains.setval

What issues does this PR fix or reference?

This allows the user to specify refresh_pillar=False

[max-arnold]

How a user (who is not familiar with the code) is supposed to discover that it is possible to pass refresh_pillar=False to these functions?

[alan-cugler]

This does not need test case since its a bug fix not a new feature request.

As for user discovery of possible key:value to pass, that can be added in a one liner to the docs.

[waynew]

This does not need test case since its a bug fix not a new feature request.

(Un)fortunately that statement is incorrect.

Tests are required for bug fixes to prevent regressions. If we can't reproduce a problem with a test case, it's likely that we haven't fully understood the problem.

Adding tests that exhibit the existing problem will at least notify us if we re-introduce that problem in the future.

[jtraub91]

The "problem" that's occurring is that pillar is refreshed when setting grains with set_val. I added the ability to pass kwargs as a "quick fix" to allow for bypassing the pillar refresh. I would argue that the bigger issue is that the default behavior of saltutil.refresh_grains is to also refresh the pillar. Changing this would likely break things for people, however..

[sagetherage]

talking to @Ch3LL if this is intended https://github.com/saltstack/salt/blob/master/salt/modules/saltutil.py#L383-L385 and the better fix would be to add refresh_pillar as a specific kwarg, and yes as @waynew stated please add a test

[DmitryKuzmenko]

@jtraub91 are you able to follow up the suggestions?

[DmitryKuzmenko]

@jtraub91 please let me know if I can help here.

[jtraub91]

@DmitryKuzmenko I can re-implement this by specifying refresh_pillar as a specific kwarg, but I still don't think this warrants a new test case. Not sure what I would even test in that scenario.

[DmitryKuzmenko]

@jtraub91 yes please if possible.
What about test, no problem with it. All you need is just to add a test to tests/unit/modules/test_grains.py that mocks saltutil.refresh_grains and checks it's called with proper refresh_pillar value.

[jtraub91]

@DmitryKuzmenko Ok. Will do.

[DmitryKuzmenko]

Thank you!

[jtraub91]

Changes have been pushed. Not sure why it's saying "unknown repository" above, so I opened a new PR to resolve this. Further discussion should ensure there (#56573) and this PR can be closed.

[DmitryKuzmenko]

@jtraub91 am I right we can close this now?

[jtraub91]

@DmitryKuzmenko Yes and let's make sure we get #56573 merged.

[DmitryKuzmenko]

I will.