[Magnesium RC] Fix bug introduced in #58238 feature flag implementation

[mlasevich]

What does this PR do?

Fix bug introduced in 9380b56 in #58238

What issues does this PR fix or reference?

Fixes bug in #58238

Previous Behavior

A typo in commit dropped a return statement from a function

New Behavior

add return statement

Merge requirements satisfied?

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

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.

[max-arnold]

@krionbsd Could you please tag this PR with the Magnesium milestone? The referenced PR was shipped in the Magnesium RC and this follow-up PR should fix the issue found while testing the RC.

[max-arnold]

The docs still contain feature flag name that differs from the actual implementation. And something needs to be improved in the unit-test so it actually checks the desired behaviour.

[myii]

@mlasevich It would be useful to fix the typos from #58238 here as well:

• Cleanup calculation of template sls/tpl context #58238 (review)

[mlasevich]

@mlasevich It would be useful to fix the typos from #58238 here as well:

• #58238 (review)

I am a bit hesitant to update those as they were not added by me, but typos are typos - so might as well update :-)

[dwoz]

@mlasevich Please address @max-arnold's comment about adding a test to actually verify this functionality.

[mlasevich]

@mlasevich Please address @max-arnold's comment about adding a test to actually verify this functionality.

Yeah, I looked at it yesterday - the only thing that was not covered was the feature flag wrapper, which is why the unit tests passed despite the bug. I wrote a unit test for feature flag stuff, but have not been able to test it as for whatever reason, nox does not work with this project for me. I'll throw together a VM this morning and get it tested and committed.

[myii]

After @max-arnold suggested the idea, I've run tests across ~85 formulas, including this fix and using the feature flag in the minion configuration, for both master and tiamat pre-salted images:

• https://saltstack-formulas.zulipchat.com/#narrow/stream/239693-CI/topic/master.2F2020-W43a
  • (Currently requires logging in such as using the GitHub integration).

Generally, all is working well. Having a problem with the tiamat jobs but that appears unrelated to this PR:

• https://travis-ci.org/github/myii/nut-formula/jobs/737010324#L1115-L1124

Testing locally, the issue is the configuration passed through to kitchen-salt:

provisioner:
  name: salt_solo
  ...
  salt_minion_extra_config:
    features:
      enable_slsvars_fixes: true

• With a dirty hack to avoid salt_minion_extra_config, I was able to confirm that this PR is also working fine on tiamat as well.

[mlasevich]

After @max-arnold suggested the idea, I've run tests across ~85 formulas, including this fix and using the feature flag in the minion configuration, for both master and tiamat pre-salted images:

I just wanted to start with unit tests. If unit tests cover functionality AND flag selection (which they do in this case now), this sort of a thing would not be a problem :-) There is some value in higher level tests, but those are more to catch deficiencies in unit tests :-)

[mlasevich]

@mlasevich Please address @max-arnold's comment about adding a test to actually verify this functionality.

Added unit tests to cover flag selection.