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

Fix complex grain #52710

Merged
merged 14 commits into from May 9, 2019
Merged

Fix complex grain #52710

merged 14 commits into from May 9, 2019

Conversation

mickenordin
Copy link
Contributor

@mickenordin mickenordin commented Apr 26, 2019

What does this PR do?

If a grain is a list of complex objects, grains.py is unable to compare the currently set grain to the potentially new one. Instead it fails with TypeError: unhashable type: 'dict' (might be something else aswell depending on what you are trying to set as a grain). This PR will make it so that a list grain that contains complex objects can be compared via the function make_hashable.

What issues does this PR fix or reference?

issue #39875

Tests written?

Yes, in tests/unit/states/test_grains.py there is a test called "test_make_hashable"

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.

This commit will make it so that grains that are lists of complex
objects will be flattened ensuring that comparisons can be made

Fixes #39875
@mickenordin mickenordin requested a review from as a code owner Apr 26, 2019
@ghost
Copy link

@ghost ghost commented Apr 26, 2019

@mickenordin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ryan-lane and @tkwilliams to be potential reviewers.

@waynew
Copy link
Contributor

@waynew waynew commented Apr 26, 2019

Hi! Thanks for the PR!

I think you either meant to make this PR against a different branch or you want to rebase your changes against develop. If you need any pointers I'm happy to help!

@mickenordin
Copy link
Contributor Author

@mickenordin mickenordin commented Apr 26, 2019

@waynew
Copy link
Contributor

@waynew waynew commented Apr 26, 2019

@mickenordin If you've just tried to merge into the wrong branch you can go to the top right and click "edit" and then change what branch you're trying to merge into (probably 2018.3 is my guess).

If that's not right, let me know where it belongs, and where you branched from, and I can help you rebase or cherry pick your changes.

@mickenordin mickenordin changed the base branch from develop to 2018.3 Apr 26, 2019
@mickenordin
Copy link
Contributor Author

@mickenordin mickenordin commented Apr 26, 2019

@waynew thank you very much, now it looks cleaner I think :)

Copy link
Contributor

@waynew waynew left a comment

Thanks for the PR! Could you update the function definition, as well as add some tests?

tests/unit/states/test_grains.py would be a good place to add (or update) tests for this.

flatten(subli, flattened)
else:
flattened.append(frozenset(subli))
return set(flattened)
Copy link
Contributor

@waynew waynew Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to just use set here? The default arg definitely should be changed, though.

I'm thinking something like:

def flatten(li, flattened=None):
    flattened = flattened or set()
    for subli in li:
        if type(subli) == list:
            flattened.update(flatten(subli))
        else:
            flattened.add(frozenset(subli))
    return flattened

I'm not entirely certain that's the best approach either, though.

Copy link
Contributor Author

@mickenordin mickenordin Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I will fix those things :)

@mickenordin
Copy link
Contributor Author

@mickenordin mickenordin commented Apr 30, 2019

@waynew thank you for you patiens and your help. I have now fixed all lint errors, added a test that uses the function and added comments and over all tried to make the code more clear and precise.

waynew
waynew approved these changes Apr 30, 2019
@mickenordin
Copy link
Contributor Author

@mickenordin mickenordin commented May 6, 2019

What is the next step for this PR? Is there a process for merging or is more review needed?

@waynew
Copy link
Contributor

@waynew waynew commented May 9, 2019

@mickenordin shouldn't be - just keep getting changes before we can merge it in 😑 I'll keep my eye out today

@waynew waynew merged commit 6946dcc into saltstack:2018.3 May 9, 2019
17 checks passed
@waynew
Copy link
Contributor

@waynew waynew commented May 9, 2019

🎉

@mickenordin mickenordin deleted the fix-complex-grain branch May 9, 2019
garethgreenaway added a commit to garethgreenaway/salt that referenced this issue Sep 19, 2019
@waynew waynew added this to PR needs port to master in PRs to port to master Oct 24, 2019
dwoz added a commit that referenced this issue Dec 23, 2019
@garethgreenaway garethgreenaway moved this from PR needs port to master to PR merged in PRs to port to master Mar 24, 2020
@garethgreenaway garethgreenaway moved this from PR merged to PR has port to master in PRs to port to master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs to port to master
  
PR has port to master
Development

Successfully merging this pull request may close these issues.

None yet

3 participants