Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No longer passes missing in list compound engine #49435

Merged

Conversation

Projects
None yet
7 participants
@brejoc
Copy link
Member

commented Aug 30, 2018

What does this PR do?

Missing targets cause a very odd behaviour in combination with a "not" statement and don't seem to be used in any other case.

Having a compound list target like not L@foominion would cause a return for foominion even though it doesn't exist. Returning an empty list for "missing" fixes that.

What issues does this PR fix or reference?

#49205

Previous Behavior

Given that there is only one minion registered called minion1.

$ salt -C 'not L@foominion' grains.get os
minion1:
    SUSE
foominion:
    Minion did not return. [No response]

New Behavior

Given that there is only one minion registered called minion1.

$ salt -C 'not L@foominion' grains.get os
minion1:
    SUSE

Tests written?

No

Commits signed with GPG?

Yes

No longer passes missing in list compound engine
Missing targets cause a very odd behaviour in combination with a
"not" statement and don't seem to be used in every other case.

Having a compound list target like "not L@foominion" would cause
a return for "foominon" even though it doesn't exist. Returning
an empty list for "missing" fixes that.

@brejoc brejoc requested a review from saltstack/team-core as a code owner Aug 30, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Aug 30, 2018

Show resolved Hide resolved salt/utils/minions.py

@gtmanfred gtmanfred requested a review from garethgreenaway Aug 30, 2018

Ignore missing minions only when excluding them with 'not'
Co-authored-by: Mihai Dinca <Mihai.Dinca@suse.com>
@brejoc

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2018

@gtmanfred I think we found the use-case you where referring to. You where probably talking about a list like L@minion1,foominion, which should also return a response for foominion even though it is not existing. We've push an update that should reflect that.

@rallytime rallytime requested a review from cachedout Aug 31, 2018

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2018

@brejoc Right now, @gtmanfred is out for a little while on leave but he should be back in a few weeks to look at this. I'm not totally positive which case he's referring to so I'd rather wait and let him respond instead of guessing. Apologies for the delay.

@brejoc

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

Perhaps @garethgreenaway can take a look here, since he is afaik the original author of the change that introduced this behavior? Sorry for the impatience, but this is blocking the CaaSP release. Thanks!

rallytime and others added some commits Sep 4, 2018

@isbm

isbm approved these changes Sep 11, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

@brejoc This is causing some tests to fail: https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-49435/5/

Can you take a look there? The matching and batch tests are related to this change.

brejoc added some commits Sep 12, 2018

Minor fix for def test_batch_run_grains_targeting
item was never "minion" without a colon, but os_grain was overwritten
anyway. So this fix does not actually affect the test outcome.
Adds check for engine before adding the additional engine argument
The additional engine argument is only intended to be used with the
_check_list_minons engine and shouldn't be added for every other engine.
@brejoc

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

@rallytime Test should be fixed now. Apart from integration/shell/test_matcher.py::MatchTest::test_grain. Looks like this was broken before.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2018

@terminalmage Since you have been in the matchers code today, you might have an opinion on this.

@rallytime rallytime requested a review from terminalmage Sep 21, 2018

@terminalmage
Copy link
Contributor

left a comment

I would wait until #48809 is merged, and then re-do this PR. The reason for this is that most of this code will no longer exist (or will exist in a different form, in a different file) once that PR is merged.

@brejoc

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2018

@terminalmage Sounds good. But are the changes from #48809 backported? Because this fixes a bug that exists down to at least 2018.3.0.

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

This pr and that one is being proposed against develop.

So you will need to open a new pr against the older release to have it be backported.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

#48809 is against fluorine as making the matchers pluggable is a new feature for fluorine.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@brejoc Where possible, please open pull requests against the oldest release branch where the bug exists: https://docs.saltstack.com/en/latest/topics/development/contributing.html#which-salt-branch

In this case though we would need two PRs due to the massive difference in how matchers are architected beginning in the Fluorine release. You can open one PR against 2018.3 and another against our fluorine branch.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 16, 2018

@gtmanfred Re-review requested.

Mike Place

@rallytime rallytime merged commit d3245b8 into saltstack:develop Oct 16, 2018

9 of 10 checks passed

jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
WIP ready for review
Details
codeclimate All good!
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.