salt icon indicating copy to clipboard operation
salt copied to clipboard

[master] Allow salt-call arguments --file-root, --pillar-root and --states-dir to be specified multiple times

Open martijnkruiten opened this issue 2 years ago • 12 comments

What does this PR do?

What issues does this PR fix or reference?

Fixes: #64486

Previous Behavior

The salt-call arguments --file-root, --pillar-root and --states-dir can each be specified only once.

New Behavior

The salt-call arguments --file-root, --pillar-root and --states-dir can each be specified multiple times.

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?

Yes

Please review Salt's Contributing Guide for best practices.

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

martijnkruiten avatar Jul 10 '23 17:07 martijnkruiten

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey. Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

welcome[bot] avatar Jul 10 '23 17:07 welcome[bot]

Hi @MKLeb, can you have a look at this MR? This looks like a great addition to make salt-call more flexible.

kees-closed avatar Oct 03 '23 12:10 kees-closed

Hi @martijnkruiten , I see you have a test for multiple file roots (I commented on that one). We would really like to see full test coverage for this PR, so are you able to add a similar style of test for multiple pillar roots and state directories?

MKLeb avatar Oct 03 '23 16:10 MKLeb

Hi @MKLeb, I'll have a look. I seem to recall that I didn't add them because these options weren't included in the test suite to begin with.

martijnkruiten avatar Oct 10 '23 13:10 martijnkruiten

I've added a test for the pillar roots that merges a dict that's defined in two different directories and then asserts that all the keys are present. What I'm stuck at, however, is writing a test for the states dirs. It seems to be sensitive to the environment that the file exists in (base vs prod), so I have to think of a different way to test this functionality.

martijnkruiten avatar Oct 10 '23 16:10 martijnkruiten

I think I'm going to need some help writing a test for --states-dir, as I'm not sure how it should be used exactly and there's not a lot of documentation available (it's not even mentioned in salt-call.rst). I thought that I could just point it to a random directory with sls files in it and that it would be able to run the states defined in those files, but it seems to be a bit more complicated than that.

Alternatively, I'm also fine with reverting the chances to that argument and going forward with the --file-root and --pillar-root options only. I'm still convinced that the change should work for --states-dir the same as it does for the other options, but I lack the knowledge to prove it in a test.

martijnkruiten avatar Oct 10 '23 19:10 martijnkruiten

@twangboy I've made all the requested changes. Please review and let me know if further adjustments are needed.

martijnkruiten avatar Aug 13 '25 17:08 martijnkruiten

@twangboy I've made sure that the pre-commit checks all pass.

martijnkruiten avatar Aug 29 '25 12:08 martijnkruiten

@twangboy is there anything else I can do to make this go through?

martijnkruiten avatar Oct 29 '25 10:10 martijnkruiten

Just need to get the tests to pass

twangboy avatar Oct 31 '25 15:10 twangboy

Just need to get the tests to pass

Is there anything that someone could do to help? I am interested in this PR.

peterupton avatar Nov 24 '25 19:11 peterupton

Once the nightlies for master branch are passing, then we can rebase and see where we're at on the tests.

twangboy avatar Nov 24 '25 19:11 twangboy