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

filter_by supports list fix #56689

Merged
merged 4 commits into from
Oct 5, 2020
Merged

filter_by supports list fix #56689

merged 4 commits into from
Oct 5, 2020

Conversation

Akm0d
Copy link
Contributor

@Akm0d Akm0d commented Apr 17, 2020

What does this PR do?

filter_by supports list

What issues does this PR fix or reference?

closes #39709

Commits signed with GPG?

Yes

Tests written

Yes

@Akm0d Akm0d requested a review from a team as a code owner April 17, 2020 21:00
@Akm0d Akm0d self-assigned this Apr 17, 2020
@ghost ghost requested review from dwoz and removed request for a team April 17, 2020 21:00
@Akm0d Akm0d changed the title fixed issue #39809 fixed issue #39709 Apr 18, 2020
@dwoz
Copy link
Contributor

dwoz commented Apr 18, 2020

@Akm0d Why is key sometimes not a str? When it isn't a str, what is it... a list?

In general, we want to avoid arguments that can have multiple types.

@Akm0d
Copy link
Contributor Author

Akm0d commented Apr 18, 2020

@Akm0d Why is key sometimes not a str? When it isn't a str, what is it... a list?

In general, we want to avoid arguments that can have multiple types.

The discussion in issue #39709 requests that filter_by support a list as a key. It could be that this change belongs further up in the filter_by function, but the line of code that errored in the issue was in this function man_shrugging

@dwoz
Copy link
Contributor

dwoz commented Apr 19, 2020

@Akm0d Why is key sometimes not a str? When it isn't a str, what is it... a list?
In general, we want to avoid arguments that can have multiple types.

The discussion in issue #39709 requests that filter_by support a list as a key. It could be that this change belongs further up in the filter_by function, but the line of code that errored in the issue was in this function man_shrugging

I think it's worth making sure key will be a consistent type.

dwoz
dwoz previously approved these changes Apr 24, 2020
@dwoz dwoz self-requested a review April 24, 2020 06:29
@sagetherage sagetherage added has-failing-test Magnesium Mg release after Na prior to Al labels May 23, 2020
@sagetherage sagetherage changed the title fixed issue #39709 filter_by supports list fix May 23, 2020
@sagetherage sagetherage removed the Magnesium Mg release after Na prior to Al label Jun 1, 2020
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Jun 10, 2020
@sagetherage
Copy link
Contributor

@Akm0d can you fix the merge conflict here, please?

@dwoz dwoz merged commit 4669f13 into saltstack:master Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grains.filter_by grain value doesn't support list
4 participants